Bug 25376

Created a proposal for this here: http://docs.google.com/Doc?id=dhs4g97m_8cwths74m

Created attachment 30936 [details] The first patch. In preparation for combining SessionStorage and LocalStorage into a single StorageManager, move the syncing bits out of LocalStorage and into its own class. The next step is to combine LocalStorageArea and SessionStorageArea into one. Then SessionStorage and LocalStorage can be combined. Note that combining them will cut down on code paths that must be proxied, eliminate virtual dispatch, and allow Chromium to (eventually) write historical sessionStorage to disk when memory is getting tight. Also remove a couple bits of cruft including code for quotas which is unnecessary (since a meta-data db is unnecessary since you can just count bytes as you read the local storage databases into memory).

Created attachment 30978 [details] Revision of the last patch Oops, a change I was making to 'contains' (which I've since undone) accidentally made it into the last patch. I think this fixes it. Would really appreciate if someone could take a look!

I'm really not at all familiar with the localstore code, so I'm not in a great position to give you a real review. (Not that I have review rights anyway:) 28 count bytes as you read the local storage databases into memory This implies to me the entire db is slurped into memory. Is that right? 65 : m_path(path.copy()) nit: is the call to .copy() really necessary 68 if (!path.isEmpty()) 69 m_syncManager = StorageSyncManager::create(m_path); nit: use the same (m_)path variable in the test and create call 115 ASSERT(origin); 116 if (!origin) 117 return String(); nit: which is it, required or not? One of them can probably go. 124 return pathByAppendingComponent(m_path, origin->databaseIdentifier() + ".localstorage"); Oh, a unique database file per origin. I see this was pre-existing code. Do you know why there is one per origin instead of one for all? Just curious. WebCore/storage/StorageSyncManager.cpp line 80: extra whitespace here

(In reply to comment #4) > I'm really not at all familiar with the localstore code, so I'm not in a great > position to give you a real review. (Not that I have review rights anyway:) > > > 28 count bytes as you read the local storage databases into memory > > This implies to me the entire db is slurped into memory. Is that right? Yes. This is how it's always been. > > 65 : m_path(path.copy()) > > nit: is the call to .copy() really necessary This is how the original code was, but I can take a look to see if it's OK to skip the .copy(). > 68 if (!path.isEmpty()) > 69 m_syncManager = StorageSyncManager::create(m_path); > > nit: use the same (m_)path variable in the test and create call Oops. Quite correct. > 115 ASSERT(origin); > 116 if (!origin) > 117 return String(); > > nit: which is it, required or not? One of them can probably go. ASSERT will tell you there's a problem in debug mode, but WebKit will crash a few lines later without the if statement. I don't know if there are actual cases where origin could be null or if this code was "just in case". Something I would like to do in the future is investigate this stuff, but I don't think it's a priority. > 124 return pathByAppendingComponent(m_path, origin->databaseIdentifier() + > ".localstorage"); > > Oh, a unique database file per origin. I see this was pre-existing code. > Do you know why there is one per origin instead of one for all? Just curious. Because there's a database per origin. > WebCore/storage/StorageSyncManager.cpp > line 80: extra whitespace here Noted. These really aren't enough changes for me to roll another patch as is, but I'll do it before this gets submitted. Thanks for reviewing, Michael.

Created attachment 31014 [details] Patch 2 in this set I'm trying to make these patches as small and self contained as possible. I'll continue rolling them out while I wait on reviews for earlier ones. I'm now using git with a branch for each one, so I should be able to keep all the layers straight.....but prompt reviews are obviously going to make this easier on everyone. This is the second patch in the set. It takes another step towards abstracting out the sync process. In this case, it's getting rid of all the 'internalGetItem' (and associated calls) and instead replaces them with one virtual function call that'll block until the import is complete. I'm pretty sure that the behavior for setItem, removeItem, and clear are NOT safe. getItem and contains do have a small optimization that is legit, but I'm don't think it helps a whole lot in practice since, if it's not present right away, it'll block until the import is complete. I've made a FIXME comment about possible optimizations. I'd like to implement this after a couple more rounds of refactoring. Unlike most svn patches, this needs to be applied with 'patch -p1' instead of 'patch -p0'. I'll create a ChangeLog and finalize this patch once the first one is reviewed. I'm putting this (and probably more soon) up mostly as a FYI.

Created attachment 31016 [details] A _slight_ ref of patch 1 A couple tiny style changes from the last patch. This is also now generated from 'git diff' which means that StorageSyncManager.* shows up just as an add rather than a copy from LocalStorage.*. This is significant because much of the code in this new file is actually unmodified...so the reviewers job is easier than it might otherwise appear. :-)

I figured it might help reviewers if I explained what I'm doing in excruciating detail. High level overview of structure: *DOMWindow code* Calls LocalStorage/SessionStorage to get a storage area Creates a Storage object and returns it to the javascript implementation *LocalStorage/SessionStorage code* Keeps track of *StorageArea objects associated with each origin Handles part of the syncing issues *StorageArea/LocalStorageArea/SessionStorageArea* Uses StorageMap to keep track of the actual data Dispatches events Handles the rest of the syncing and concurrency issues *StorageMap* Keeps the string -> string map that is DOM Storage Where I'm headed: I want to reorganize everything by function. I want to have the event dispatching code in one place. I want the syncing code in one place. And I want the data mapping in one place. To do this, I intend to pull the factory part of LocalStorage/SessionStorage into a StorageManager. I want to pull the syncing part of LocalStorage into a StorageSyncManager. I want to pull LocalStorageArea syncing code either into that manager or (more likely) a StorageAreaSync class. I want to move the event dispatching into a StorageEventDispatcher class. This is important for setting up proxies across processes. For the StorageArea, StorageManager, and their helpers, the "real" implementation will live in Chromium's browser process (the main, unsandboxed one). For StorageEventDispatcher, the "real" implementation will live in the renderer process and its proxy will live in the browser process. If this doesn't make sense, let me know and I'll try to flesh it out further (or update the design doc :-) As of right now, here's what I have planned in terms of patches. This will probably change, but hopefully it can give you a high level overview of where I'm going with this: 1: Pull LocalStorage sync code into its own class. 2: Clean up the LocalStorageCode for handling the initial import. 3: Pull the rest of the sync code out of localStorageArea. 4: Pull the event dispatching code out of *StorageArea. 5: Combine LocalStorageArea and SessionStorageArea. Combine LocalStorage and SessionStorage into StorageManager. All of the heavy lifting will have already been done in other patches. 6: Made the actual proxy split. Once again, if you have questions PLEASE ASK! I really want to keep moving forward on this, so I'll take whatever time necessary to explain what I'm doing. :-)

Created attachment 31024 [details] Patch 3 in this set Patch for item #3. (Moving the rest of the syncing code out of LocalStorageArea.) Once #1 and #2 are in, I'll make a ChangeLog for this one and add the new file to the visual studio project, make file, etc.

Created attachment 31085 [details] Patch 4 in this set Combine LocalStorageArea and SessionStorageArea into StorageArea. Next up, combine LocalStorage and SessionStorage into StorageManager.

Created attachment 31193 [details] Patch 5 in this set Merge SessionStorage and LocalStorage into a single StorageNamespace class (since that's essentially what they both were to begin with). The next step is to add a proxy for StorageNamespace and StorageArea.

Why do we need some complicated multiprocess sync code in all local storage code when there is no way to build the WebKit tree in a multiprocess form?

Chromium is a WebKit based browser that makes use of a multi-process based architecture. You can find more details here: http://dev.chromium.org/developers/design-documents/multi-process-architecture Please take a look at the patches I've posted so far. I think the architecture is actually simpler after them. Stuff like syncing code is broken out into its own area. What remained could then be combined and some virtual dispatch and tricks like having an "internalFunctionFoo" could be eliminated. As for "some complicated multiprocess sync code," I actually haven't posted any multi-process specific code yet. It will make things more complex, but it's important so that Chromium and the rest of WebKit can continue sharing as much code as possible. A couple of us met with a bunch of Apple guys (all of them in the cc list except for Maciej) to talk through the general strategy before. One of the things we talked about was moving in the direction where it's easier for others to use WebKit in a multi-process manner. We've also discussed some of the details on the webkit-dev mailing list. (I can dig up some of the thread names if that'd be helpful.) I think the benefits of sharing code will outweigh the negatives of added complexity. And I think the 5 patches I've posted so far will help reduce this added complexity.

Created attachment 31389 [details] patch 1 - v3 Synced the repo and generated the patch with svn-create-patch. This should help the readability of StorageSyncManager.* (since it started as a copy of LocalStorage.*).

Created attachment 31395 [details] patch 1 - v4 Added ENABLE(DOM_STORAGE) guards around the new files.

Note that I've generated newer versions of the other patches (after syncing with head and adding guards) but they're almost the same, so I'm not going to bother uploading them unless someone expresses any amount of interest.

Comment on attachment 31395 [details] patch 1 - v4 This refactoring seems like a good idea to me. Reading over this change, I spotted some stylistic things you'll want to fix. Marking R- on account of those. > Index: WebCore/storage/LocalStorage.cpp ... > LocalStorage::LocalStorage(const String& path) > : m_path(path.copy()) > + , m_syncManager(0) nit: no need for this line since RefPtr's default constructor is equivalent. it is commonplace in webcore to exclude lines like this. > { > - // If the path is empty, we know we're never going to be using the thread for anything, so don't start it. > - // In the future, we might also want to consider removing it from the DOM in that case - <rdar://problem/5960470> > - if (path.isEmpty()) > - return; Can you comment on why it makes sense to delete this comment? > @@ -92,54 +88,18 @@ PassRefPtr<StorageArea> LocalStorage::st ... > - storageArea = LocalStorageArea::create(origin, this); > + nit: ^^^ looks like some extraneous white spaces on that new line. > Index: WebCore/storage/LocalStorageArea.cpp ... > void LocalStorageArea::scheduleFinalSync() > { > + ASSERT(isMainThread()); > + if (!m_syncManager) > + return; > + nit: ^^^ looks like some extraneous white spaces on that new line. > Index: WebCore/storage/StorageSyncManager.cpp ... > +void StorageSyncManager::close() ... > + nit: ^^^ looks like some extraneous white spaces on that new line. > +bool StorageSyncManager::scheduleImport(PassRefPtr<LocalStorageArea> area) > { > ASSERT(isMainThread()); > - > + nit: ^^^ looks like some extraneous white spaces on that new line. > +void StorageSyncManager::scheduleSync(PassRefPtr<LocalStorageArea> area) > { > ASSERT(isMainThread()); > + nit: ^^^ looks like some extraneous white spaces on that new line. > if (m_thread) > m_thread->scheduleSync(area); > } > Index: WebCore/storage/StorageSyncManager.h ... > + class StorageSyncManager: public ThreadSafeShared<StorageSyncManager> { nit: need a space before the ":"

Created attachment 31504 [details] patch 1 - v5 (In reply to comment #17) > > { > > - // If the path is empty, we know we're never going to be using the thread for anything, so don't start it. > > - // In the future, we might also want to consider removing it from the DOM in that case - <rdar://problem/5960470> > > - if (path.isEmpty()) > > - return; > > Can you comment on why it makes sense to delete this comment? The comment just doesn't really fit in anywhere now. The patch check is gone since the manager now handles that stuff and the comment doesn't reflect the current thinking. (https://bugs.webkit.org/show_bug.cgi?id=25894) Once the re-factoring is done, it'll be easier to fix the behavior for WebKit while allowing browsers (with different philosophies) to opt out. Doing so earlier will just make the diffs a lot more messy. And given that this has been a problem since the beginning, I don't think it's a big deal to wait a bit on it. :-) > > @@ -92,54 +88,18 @@ PassRefPtr<StorageArea> LocalStorage::st > ... > > - storageArea = LocalStorageArea::create(origin, this); > > + > > nit: ^^^ looks like some extraneous white spaces on that new line. The spacing is as it was originally...I just removed an extra tab it appears. I think the spacing is proper as it is now. > > > > Index: WebCore/storage/LocalStorageArea.cpp > ... > > void LocalStorageArea::scheduleFinalSync() > > { > > + ASSERT(isMainThread()); > > + if (!m_syncManager) > > + return; > > + > > nit: ^^^ looks like some extraneous white spaces on that new line. I guess I could remove it, but that code is logically grouped together. It's doing sanity checks and deciding if it should bail completely...essentially testing assumptions which are made throughout the rest of the function. I don't like how it reads when you remove the space, but I'll do it if you insist. > > > > Index: WebCore/storage/StorageSyncManager.cpp > ... > > +void StorageSyncManager::close() > ... > > + > > nit: ^^^ looks like some extraneous white spaces on that new line. > > > > +bool StorageSyncManager::scheduleImport(PassRefPtr<LocalStorageArea> area) > > { > > ASSERT(isMainThread()); > > - > > + > > nit: ^^^ looks like some extraneous white spaces on that new line. > > > > +void StorageSyncManager::scheduleSync(PassRefPtr<LocalStorageArea> area) > > { > > ASSERT(isMainThread()); > > + > > nit: ^^^ looks like some extraneous white spaces on that new line. Same as above...I'm putting a space between where I'm testing assumptions and where the real code is. This was done often in the code I started with, but I did add in a bunch of spaces to make it all consistent. I went ahead and made all the changes you asked for because making progress is more important than these nits, but I think they've decreased readability and consistency.

Created attachment 31571 [details] patch 1 - v6 Realized I misinterpreted Darin's comments. Added back in the new lines without the trailing spaces. I'm pretty sure this is ready to +. :-)

Sorry I've been absent through this whole process - I'd like to take a thorough look before this lands, which I am doing right now. I'll be more available from here on out!

> Sorry I've been absent through this whole process - I'd like to take a thorough > look before this lands, which I am doing right now. I'll be more available > from here on out! Looks like I just missed your message :( I went ahead and committed this patch as http://trac.webkit.org/changeset/44959. I'm happy to revert or revise as you suggest.

If it's ok, I'd like to keep this in for now (so that I can prepare the next patch). These patches are really affecting my ability to get LocalStorage working in Chromium, and that in turn is holding up other people/projects (like extensions). I really did want you feedback before we started checking anything in, but I felt somewhat backed into a corner. There still are plenty of other patches to fix any issues you might have with the first patch, and we can always back the first out if there's some major architectural flaw. As I've mentioned (via private mail) I'm more than happy to come by Apple to talk these patches through in person, on IRC, or whatever. I know in person isn't normal, but it'd probably be most efficient (and an excuse for me to ride my motorcycle :-).

None of that was the issue - timing and availability was all. Please keep me CC'ed on future patches going forward.

Created attachment 31685 [details] patch 2 - v2 Simplify the interaction between LocalStorageArea/SessionStorageArea and StorageArea by creating a "blockUntilImportComplete()" function rather than bouncing back and forth between the child and parent classes in a somewhat unintuitive manner. This patch also paves the way for LocalStorageArea and SessionStorageArea being merged into one. It's part of several in a set which are separating syncing (to disk) code from the rest of the implementation so that local storage and session storage's code can be unified.

In general we try not to keep reusing the same back for a sequence of patches. One bug -> one patch makes it possible to track changes. Could we try opening a new bug for the next patch(es) in the sequence please?

So f(In reply to comment #23) > Please keep me CC'ed on future patches going forward. Pretty much all the work is happening in this bug, but I'll definitely keep that in mind in the future. Here are the only bugs about LocalStorage that _might_ have been of interest: https://bugs.webkit.org/show_bug.cgi?id=26356 https://bugs.webkit.org/show_bug.cgi?id=25970 These are even less related: https://bugs.webkit.org/show_bug.cgi?id=26154 https://bugs.webkit.org/show_bug.cgi?id=26180 P.S. It'd be awesome if you can take a look at the next patch today. (It's pretty straightforward.)

Jeremy, stop monopolizing bradee-oh's attention! :) We need him for this bug: https://bugs.webkit.org/show_bug.cgi?id=26054

Moved discussion of the second patch to a new thread as requested: https://bugs.webkit.org/show_bug.cgi?id=26629 I'm leaving patches 3-5 here for reference (until I cut a "real" version of each).

Adding the broken out bugs as dependencies.

So is this bug still waiting to be landed? Or should it be closed?