Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2861473002:
Clear up session only storage on localstorage shutdown (Closed)
|
Created:
3 years, 7 months ago by Marijn Kruisselbrink Modified:
3 years, 7 months ago Reviewers:
michaeln CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), darin-cc_chromium.org, jam, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionClear up session only storage on localstorage shutdown
This changes LocalStorageContextMojo to manage its own destruction, to allow
it to outlive the DOMStorageContextWrapper that owns is, in order to finish
cleaning up on shutdown.
This also moves LocalStorageContextMojo away from the UI thread to the IO
thread, because shutdown sequence won't let the UI thread make progress.
Ultimately this should probably use a task scheduler managed SequencedTaskRunner,
but currently mojo doesn't work on sequenced task runners yet, and introducing
an entire new thread for this seems too heavy weight.
BUG=586194
Review-Url: https://codereview.chromium.org/2861473002
Cr-Commit-Position: refs/heads/master@{#473073}
Committed: https://chromium.googlesource.com/chromium/src/+/5f4cb613f6dfaa504efc346f278899ae5d995187
Patch Set 1 #Patch Set 2 : test and nicer #
Total comments: 10
Patch Set 3 : address comments #Patch Set 4 : rebase #
Total comments: 6
Patch Set 5 : address comments #
Messages
Total messages: 42 (33 generated)
The CQ bit was checked by mek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== WIP mojo localstorage shutdown behavior BUG= ========== to ========== WIP mojo localstorage shutdown behavior One requirement for this is moving LocalStorageContextMojo away from the UI thread to the IO thread, because shutdown sequence won't let the UI thread make progress. BUG= ==========
The CQ bit was checked by mek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by mek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by mek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== WIP mojo localstorage shutdown behavior One requirement for this is moving LocalStorageContextMojo away from the UI thread to the IO thread, because shutdown sequence won't let the UI thread make progress. BUG= ========== to ========== Clear up session only storage on localstorage shutdown This makes LocalStorageContextMojo ref-counted, to be able to outlive the DOMStorageContextWrapper that owns is, in order to finish cleaning up on shutdown. This also moves LocalStorageContextMojo away from the UI thread to the IO thread, because shutdown sequence won't let the UI thread make progress. Ultimately this should probably use a task scheduler managed SequencedTaskRunner, but currently mojo doesn't work on sequenced task runners yet, and introducing an entire new thread for this seems too heavy weight. BUG=586194 ==========
mek@chromium.org changed reviewers: + michaeln@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2861473002/diff/60001/content/browser/dom_sto... File content/browser/dom_storage/dom_storage_browsertest.cc (right): https://codereview.chromium.org/2861473002/diff/60001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_browsertest.cc:80: base::RunLoop().RunUntilIdle(); I'm not sure what the RunUntilIdle() here accomplished before, but i don't think it's applicable anymore now that the mojo interface is running on a different thread? Can this line be deleted? https://codereview.chromium.org/2861473002/diff/60001/content/browser/dom_sto... File content/browser/dom_storage/local_storage_context_mojo.cc (right): https://codereview.chromium.org/2861473002/diff/60001/content/browser/dom_sto... content/browser/dom_storage/local_storage_context_mojo.cc:289: if (connection_state_ != CONNECTION_FINISHED) { Similar to the NO_CONNECTION case, maybe we don't need to go thru the shutdown sequence here either? If we're shutting down prior to being connected, either the connection was taking forever to happen or the browsing session is super short, both are exceptional cases in one way or another. I'd vote to try to do less here and slam the connection state SHUTDOWN. 670 is_shutdown_ = true; 671 file_system_.reset(); 672 directory_.reset(); 673 leveldb_service_.reset(); 674 level_db_wrappers_.clear(); 675 database_.reset(); 676 connector_.reset(); Would it work to just pull the plug like that? https://codereview.chromium.org/2861473002/diff/60001/content/browser/dom_sto... content/browser/dom_storage/local_storage_context_mojo.cc:645: url::Origin origin(info.origin); i see you've copied the behavior for the sqlite impl, not for this cl, but i'm not sure this properly accounts for suborigins? https://codereview.chromium.org/2861473002/diff/60001/content/browser/dom_sto... File content/browser/dom_storage/local_storage_context_mojo.h (right): https://codereview.chromium.org/2861473002/diff/60001/content/browser/dom_sto... content/browser/dom_storage/local_storage_context_mojo.h:32: // for now). Describe the thread access pattern in the class comment, created on main thread but used exclusively on the IO thread. https://codereview.chromium.org/2861473002/diff/60001/content/browser/dom_sto... content/browser/dom_storage/local_storage_context_mojo.h:34: : public base::RefCountedThreadSafe<LocalStorageContextMojo> { I'd rather avoid refcounting if we can, but i see Shutdown() is async so it's difficult to avoid w/o something odd like making Shutdown() delete itself upon completion. That might not be awful because the only way to delete the object would be to go thru Shutdown() which we don't want to skip, but it's a little odd? If you were to go that route, call it something odd like ShutdownAndDelete()
Also, I think StorageParitionImpl::OpenLocalStorage running on the main thread is probably also a problem for shutdown on someplatforms. I think as tabs are closing on CrOS, the main thread is not making progess. Any renderer that attempts to open it's local storage while that's happening will block and prevent the tab from closing. I think it'd be better to also move that to the IO thread but that's a little trickier since the StoragePartionImpl class lives only on the main thread.
The CQ bit was checked by mek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/03 at 21:33:40, michaeln wrote: > Also, I think StorageParitionImpl::OpenLocalStorage running on the main thread is probably also a problem for shutdown on someplatforms. I think as tabs are closing on CrOS, the main thread is not making progess. Any renderer that attempts to open it's local storage while that's happening will block and prevent the tab from closing. > > I think it'd be better to also move that to the IO thread but that's a little trickier since the StoragePartionImpl class lives only on the main thread. Yeah, definitely seems like it might make sense to do so. But then the question just moves one API level up, to the point where the connection to the StoragePartition is made (although maybe these days that doesn't require progress on the UI thread anymore, not sure how BinderRegistry works and in general how all that currently works in mojo). https://codereview.chromium.org/2861473002/diff/60001/content/browser/dom_sto... File content/browser/dom_storage/dom_storage_browsertest.cc (right): https://codereview.chromium.org/2861473002/diff/60001/content/browser/dom_sto... content/browser/dom_storage/dom_storage_browsertest.cc:80: base::RunLoop().RunUntilIdle(); On 2017/05/03 at 21:01:29, michaeln wrote: > I'm not sure what the RunUntilIdle() here accomplished before, but i don't think it's applicable anymore now that the mojo interface is running on a different thread? > > Can this line be deleted? Hmm, yeah, good point... probably all of this flush method can actually be deleted now the shutdown behavior is implemented, as this was only used to (attempt to) make sure that data got flushed properly on browser shutdown... https://codereview.chromium.org/2861473002/diff/60001/content/browser/dom_sto... File content/browser/dom_storage/local_storage_context_mojo.cc (right): https://codereview.chromium.org/2861473002/diff/60001/content/browser/dom_sto... content/browser/dom_storage/local_storage_context_mojo.cc:289: if (connection_state_ != CONNECTION_FINISHED) { On 2017/05/03 at 21:01:29, michaeln wrote: > Similar to the NO_CONNECTION case, maybe we don't need to go thru the shutdown sequence here either? If we're shutting down prior to being connected, either the connection was taking forever to happen or the browsing session is super short, both are exceptional cases in one way or another. I'd vote to try to do less here and slam the connection state SHUTDOWN. Yeah, agreed that it makes sense to just immediately self-destruct when no connection has been made yet. Incidentally I think that might also fix some other (unrelated) issues I was having in a different CL... https://codereview.chromium.org/2861473002/diff/60001/content/browser/dom_sto... content/browser/dom_storage/local_storage_context_mojo.cc:645: url::Origin origin(info.origin); On 2017/05/03 at 21:01:29, michaeln wrote: > i see you've copied the behavior for the sqlite impl, not for this cl, but i'm not sure this properly accounts for suborigins? Not sure.. but I imagine properly accounting for suborigins should happen inside SpecialStoragePolicy's IsStorageProtected and IsStorageSessionOnly methods, after which this should automatically do the right thing. https://codereview.chromium.org/2861473002/diff/60001/content/browser/dom_sto... File content/browser/dom_storage/local_storage_context_mojo.h (right): https://codereview.chromium.org/2861473002/diff/60001/content/browser/dom_sto... content/browser/dom_storage/local_storage_context_mojo.h:32: // for now). On 2017/05/03 at 21:01:29, michaeln wrote: > Describe the thread access pattern in the class comment, created on main thread but used exclusively on the IO thread. Done https://codereview.chromium.org/2861473002/diff/60001/content/browser/dom_sto... content/browser/dom_storage/local_storage_context_mojo.h:34: : public base::RefCountedThreadSafe<LocalStorageContextMojo> { On 2017/05/03 at 21:01:29, michaeln wrote: > I'd rather avoid refcounting if we can, but i see Shutdown() is async so it's difficult to avoid w/o something odd like making Shutdown() delete itself upon completion. That might not be awful because the only way to delete the object would be to go thru Shutdown() which we don't want to skip, but it's a little odd? If you were to go that route, call it something odd like ShutdownAndDelete() Went that route. Seems reasonable, but somewhat unfortunate that now it's easier to forget to actually shutdown and delete the class. Could be solved by wrapping LocalStorageContextMojo instances in a unique_ptr with custom deleter or something, but that also would be tricky because of the need for the deleter to actually post a task to the correct task runner to delete the instance. Anyway outside of test code DOMStorageContextWrapper is the only place that has an instance of this class, and I did add a DCHECK there to make sure it always shuts down this class before the wrapper is destructed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Clear up session only storage on localstorage shutdown This makes LocalStorageContextMojo ref-counted, to be able to outlive the DOMStorageContextWrapper that owns is, in order to finish cleaning up on shutdown. This also moves LocalStorageContextMojo away from the UI thread to the IO thread, because shutdown sequence won't let the UI thread make progress. Ultimately this should probably use a task scheduler managed SequencedTaskRunner, but currently mojo doesn't work on sequenced task runners yet, and introducing an entire new thread for this seems too heavy weight. BUG=586194 ========== to ========== Clear up session only storage on localstorage shutdown This changes LocalStorageContextMojo to manage its own destruction, to allow it to outlive the DOMStorageContextWrapper that owns is, in order to finish cleaning up on shutdown. This also moves LocalStorageContextMojo away from the UI thread to the IO thread, because shutdown sequence won't let the UI thread make progress. Ultimately this should probably use a task scheduler managed SequencedTaskRunner, but currently mojo doesn't work on sequenced task runners yet, and introducing an entire new thread for this seems too heavy weight. BUG=586194 ==========
The CQ bit was checked by mek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
michaeln: ping, now you're back?
lgtm! https://codereview.chromium.org/2861473002/diff/100001/content/browser/dom_st... File content/browser/dom_storage/dom_storage_context_wrapper.h (left): https://codereview.chromium.org/2861473002/diff/100001/content/browser/dom_st... content/browser/dom_storage/dom_storage_context_wrapper.h:103: std::unique_ptr<LocalStorageContextMojo> mojo_state_; maybe continue using std::unique_ptr<> to document ownership even though we won't be using the dtor for destruction oh, nevermind, i see, the dtor is private, maybe comment about the ownership here? https://codereview.chromium.org/2861473002/diff/100001/content/browser/dom_st... File content/browser/dom_storage/local_storage_context_mojo.h (right): https://codereview.chromium.org/2861473002/diff/100001/content/browser/dom_st... content/browser/dom_storage/local_storage_context_mojo.h:47: storage::SpecialStoragePolicy* special_storage_policy); i think the project prefers passing scoped_refptr<T> https://codereview.chromium.org/2861473002/diff/100001/content/browser/dom_st... content/browser/dom_storage/local_storage_context_mojo.h:119: void OnShutdownCommitComplete(leveldb::mojom::DatabaseError error); since we sometimes don't commit, maybe just OnShutdownComplete(err)
The CQ bit was checked by mek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2861473002/diff/100001/content/browser/dom_st... File content/browser/dom_storage/dom_storage_context_wrapper.h (left): https://codereview.chromium.org/2861473002/diff/100001/content/browser/dom_st... content/browser/dom_storage/dom_storage_context_wrapper.h:103: std::unique_ptr<LocalStorageContextMojo> mojo_state_; On 2017/05/18 at 23:55:39, michaeln wrote: > maybe continue using std::unique_ptr<> to document ownership even though we won't be using the dtor for destruction > > oh, nevermind, i see, the dtor is private, maybe comment about the ownership here? It definitely would be cleanest to somehow have a std::unique_ptr with a custom deleter that posts a task to the mojo_task_runner_ to call ShutdownAndDelete. But actually doing that seemed to involve so much code and make things in general a lot more confusing (due to the need of the deleter to actually know what task runner to post a task to), so it seemed simpler to just keep it as a raw pointer, since it's not like random code will ever end up creating LocalStorageContextMojo instances... Added more of a comment explaining the ownership at least. https://codereview.chromium.org/2861473002/diff/100001/content/browser/dom_st... File content/browser/dom_storage/local_storage_context_mojo.h (right): https://codereview.chromium.org/2861473002/diff/100001/content/browser/dom_st... content/browser/dom_storage/local_storage_context_mojo.h:47: storage::SpecialStoragePolicy* special_storage_policy); On 2017/05/18 at 23:55:39, michaeln wrote: > i think the project prefers passing scoped_refptr<T> yes, good point. fixed https://codereview.chromium.org/2861473002/diff/100001/content/browser/dom_st... content/browser/dom_storage/local_storage_context_mojo.h:119: void OnShutdownCommitComplete(leveldb::mojom::DatabaseError error); On 2017/05/18 at 23:55:39, michaeln wrote: > since we sometimes don't commit, maybe just OnShutdownComplete(err) Done
The CQ bit was unchecked by mek@chromium.org
The CQ bit was checked by mek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org Link to the patchset: https://codereview.chromium.org/2861473002/#ps120001 (title: "address comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495155032279030,
"parent_rev": "9b781c354432f97658390c848359d2d70ceb1cb5", "commit_rev":
"5f4cb613f6dfaa504efc346f278899ae5d995187"}
Message was sent while issue was closed.
Description was changed from ========== Clear up session only storage on localstorage shutdown This changes LocalStorageContextMojo to manage its own destruction, to allow it to outlive the DOMStorageContextWrapper that owns is, in order to finish cleaning up on shutdown. This also moves LocalStorageContextMojo away from the UI thread to the IO thread, because shutdown sequence won't let the UI thread make progress. Ultimately this should probably use a task scheduler managed SequencedTaskRunner, but currently mojo doesn't work on sequenced task runners yet, and introducing an entire new thread for this seems too heavy weight. BUG=586194 ========== to ========== Clear up session only storage on localstorage shutdown This changes LocalStorageContextMojo to manage its own destruction, to allow it to outlive the DOMStorageContextWrapper that owns is, in order to finish cleaning up on shutdown. This also moves LocalStorageContextMojo away from the UI thread to the IO thread, because shutdown sequence won't let the UI thread make progress. Ultimately this should probably use a task scheduler managed SequencedTaskRunner, but currently mojo doesn't work on sequenced task runners yet, and introducing an entire new thread for this seems too heavy weight. BUG=586194 Review-Url: https://codereview.chromium.org/2861473002 Cr-Commit-Position: refs/heads/master@{#473073} Committed: https://chromium.googlesource.com/chromium/src/+/5f4cb613f6dfaa504efc346f2788... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/5f4cb613f6dfaa504efc346f2788... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
