Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
852463002:
Keep track of ServiceWorkerContext's BrowserContext and expose it. (Closed)
|
Created:
5 years, 11 months ago by mlamouri (slow - plz ping) Modified:
5 years, 11 months ago CC:
chromium-reviews, darin-cc_chromium.org, horo+watch_chromium.org, jam, jsbell, kinuko+serviceworker, nhiroki, serviceworker-reviews, tzik Base URL:
https://chromium.googlesource.com/chromium/src.git@nullptr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionKeep track of ServiceWorkerContext's BrowserContext and expose it.
This is happening in three steps:
1. StoragePartitionImpl keeps track of its associated BrowserContext.
2. ServiceWorkerContextWrapper keeps track of its associated StoragePartitionImpl.
3. ServiceWorkerContextCore can go up to the BrowserContext.
BUG=437151
Committed: https://crrev.com/6cdca919f433ffd0268b338fac478ef0cdea9949
Cr-Commit-Position: refs/heads/master@{#311681}
Patch Set 1 #
Total comments: 3
Patch Set 2 : #
Total comments: 11
Patch Set 3 : review comments #
Total comments: 1
Patch Set 4 : oups #
Total comments: 2
Patch Set 5 : remove include #
Messages
Total messages: 28 (8 generated)
mlamouri@chromium.org changed reviewers: + michaeln@chromium.org
https://codereview.chromium.org/852463002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/852463002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_core.h:199: BrowserContext* GetBrowserContext() const; I don't think this getter should be here. ServiceWorkerContext core lives on the IO thread and BrowserContext lives on the UI thread. When its valid to call the getter, its not valid to invoke BrowserContext methods and vice versa. What are you trying to do that this change is an attempt to help with? https://codereview.chromium.org/852463002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_context_wrapper.h (right): https://codereview.chromium.org/852463002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.h:91: BrowserContext* browser_context() const { return browser_context_; } Ah... I see why you're wanting to add this, to traverse back up the UI thread object containment hierarchy, wrapper->(partition->partitionmap)->browsercontext, for window opening purposes that need the browsercontext. The need to do that makes sense but how it's currently done in the CL doesn't (for reasons stated in other comments). I'd vote for something like this... 1. The wrapper has a StoragePartition* ptr and a getter. The getter DCHECKS it's only called on the UI thread. The storeage_partition_ data member is cleared in the Shutdown() method. 2. The StoragePartition class has a virtual getter for it's BrowserContext which may return null during shutdown. The ptr is cleared in ~StoragePartitionImpl prior to calling the various Shutdown() methods. I think lets you traverse up to the browsercontext safely. wdyt? https://codereview.chromium.org/852463002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.h:135: // BrowserContext associated with the instance. Can be null during shutdown. where does it get set to nullptr?
Michael, I've applied your review comments. I have however a quick question about them. I wonder why you insisted that all the pointers are set to null explicitly. My understanding is that the BrowserContext will be destroyed after the entire tree of object it is owning (StoragePartitionMap -> StoragePartition -> ServiceWorkerContext*). Is there something we are preventing by setting those pointers to null?
On 2015/01/13 at 14:40:27, Mounir Lamouri wrote: > Michael, I've applied your review comments. > > I have however a quick question about them. I wonder why you insisted that all the pointers are set to null explicitly. My understanding is that the BrowserContext will be destroyed after the entire tree of object it is owning (StoragePartitionMap -> StoragePartition -> ServiceWorkerContext*). Is there something we are preventing by setting those pointers to null? I have another issue actually. |context_| being a WeakPtr, I can't use it from the UI thread, which means that I can't call GetBrowserContext() from the UI thread. I could get the BrowserContext* before posting the task to the UI thread but the pointer might end up being invalid at that point, right?
On 2015/01/13 14:40:27, Mounir Lamouri wrote: > Michael, I've applied your review comments. > > I have however a quick question about them. I wonder why you insisted that all > the pointers are set to null explicitly. My understanding is that the > BrowserContext will be destroyed after the entire tree of object it is owning > (StoragePartitionMap -> StoragePartition -> ServiceWorkerContext*). Is there > something we are preventing by setting those pointers to null? The wrapper is a refcounted class so it definitely can outlive the profile/storagepartition. The core is owned by the wrapper and can also outlive the partition. The core has io thread affinity and the partition ui thread affinity, so i think the relative order of their destruction is completely racey. The refcounted classes (like swcontextwrapper) 'kinda owned' by the partition can't assume the browsercontext exists so nulling out the backptrs prior to invoking shutdown methods doesn't burden them with any new logic that wouldn't be needed otherwise. But more significantly it also helps prevents them from invoking methods of BrowserContext while inside of ~BrowserContext (which is where partitions are deleted). Does that make sense?
> I have another issue actually. |context_| being a WeakPtr, I can't use it from i need more context (haha), i have no clue what 'context_' data member you're referring to. i'll take a look at the current cl?
On 2015/01/13 at 20:18:41, michaeln wrote: > > I have another issue actually. |context_| being a WeakPtr, I can't use it from > > i need more context (haha), i have no clue what 'context_' data member you're referring to. i'll take a look at the current cl? ServiceWorkerVersion is the class that will handle the OpenWindow call. It has a weak ref on context_ which is a ServiceWorkerContextCore. This weak ptr is bound to the IO thread.
https://codereview.chromium.org/852463002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/852463002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_core.h:199: // This MUST be called in the UI thread. I think this class can remain very strictly single threaded. The wrapper can have a getter like this for use on the UI thread, and this class can expose a wrapper() accessor instead. Callers on the IO thread can safely PostTask a reference to the wrapper over to the UI thread. https://codereview.chromium.org/852463002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_context_wrapper.h (right): https://codereview.chromium.org/852463002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_wrapper.h:66: // The StoragePartition should only be used on the UI thread. Also say that it may be null before/during init and during/after shutdown. https://codereview.chromium.org/852463002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_wrapper.h:139: // WeakPtr to the StoragePartitionImpl owning |this|. Actual use of the WeakPtr<> template here might help catch programming errors that try to access it on the wrong thread. https://codereview.chromium.org/852463002/diff/20001/content/browser/storage_... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/852463002/diff/20001/content/browser/storage_... content/browser/storage_partition_impl.cc:473: // Init() requires |storage_partition| so is done after the object is created. To avoid having to alter construction ordering, it may be cleaner to have a separate set_storage_partition(). It's hard to know if anything currently depends on this ordering. https://codereview.chromium.org/852463002/diff/20001/content/browser/storage_... File content/browser/storage_partition_impl.h (right): https://codereview.chromium.org/852463002/diff/20001/content/browser/storage_... content/browser/storage_partition_impl.h:175: // Weak pointer that should always be valid. The BrowserContext owns the "Raw pointer" is more accurate since it's not a WeakPtr<>.
Comments applied. PTAL. https://codereview.chromium.org/852463002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/852463002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_core.h:199: // This MUST be called in the UI thread. On 2015/01/13 at 21:27:32, michaeln wrote: > I think this class can remain very strictly single threaded. The wrapper can have a getter like this for use on the UI thread, and this class can expose a wrapper() accessor instead. Callers on the IO thread can safely PostTask a reference to the wrapper over to the UI thread. Ok. Sounds good. I was assuming SWContextCore wasn't exposing the SWContextWrapper for some reasons but I guess I should have asked ;) https://codereview.chromium.org/852463002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_context_wrapper.h (right): https://codereview.chromium.org/852463002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_wrapper.h:66: // The StoragePartition should only be used on the UI thread. On 2015/01/13 at 21:27:32, michaeln wrote: > Also say that it may be null before/during init and during/after shutdown. Done. https://codereview.chromium.org/852463002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_wrapper.h:139: // WeakPtr to the StoragePartitionImpl owning |this|. On 2015/01/13 at 21:27:32, michaeln wrote: > Actual use of the WeakPtr<> template here might help catch programming errors that try to access it on the wrong thread. I would prefer not to abuse WeakPtr<> in order to simply have a thread safe object unless this is a common pattern already? Would that be interesting to add such a wrapper in a follow-up? ie. something that would guarantee that object X is only used on thread Y. https://codereview.chromium.org/852463002/diff/20001/content/browser/storage_... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/852463002/diff/20001/content/browser/storage_... content/browser/storage_partition_impl.cc:473: // Init() requires |storage_partition| so is done after the object is created. On 2015/01/13 at 21:27:32, michaeln wrote: > To avoid having to alter construction ordering, it may be cleaner to have a separate set_storage_partition(). It's hard to know if anything currently depends on this ordering. Ok. I guess we will never know if there are some side effects ;) https://codereview.chromium.org/852463002/diff/20001/content/browser/storage_... File content/browser/storage_partition_impl.h (right): https://codereview.chromium.org/852463002/diff/20001/content/browser/storage_... content/browser/storage_partition_impl.h:175: // Weak pointer that should always be valid. The BrowserContext owns the On 2015/01/13 at 21:27:32, michaeln wrote: > "Raw pointer" is more accurate since it's not a WeakPtr<>. Done.
lgtm https://codereview.chromium.org/852463002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/852463002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_core.h:199: // This MUST be called in the UI thread. > Ok. Sounds good. I was assuming SWContextCore wasn't exposing the > SWContextWrapper for some reasons but I guess I should have asked ;) The wrapper() was intentionally not exposed to help avoid the introduction of odd/uneeded dependencies on stuff with affinity to other threads. The clients.open function requires exactly that so it makes sense to add it now.
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/852463002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mlamouri@chromium.org changed reviewers: + avi@chromium.org
avi@, could you have a look at the changes in the following files: - content/browser/storage_partition_impl.* Thanks :)
https://codereview.chromium.org/852463002/diff/40001/content/browser/service_... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/852463002/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_context_wrapper.cc:77: is_incognito_(false) { ooops, wait, please initialize storage_partition_(nullptr) here
mlamouri@chromium.org changed reviewers: + jochen@chromium.org
mlamouri@chromium.org changed required reviewers: + michaeln@chromium.org
Oups, sorry about that :) +jochen@ in case of he can review the changes in storage_partition_impl.* in EMEA time.
jochen@chromium.org changed required reviewers: - michaeln@chromium.org
lgtm https://codereview.chromium.org/852463002/diff/60001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/852463002/diff/60001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:25: #include "content/browser/storage_partition_impl.h" why the include?
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/852463002/80001
https://codereview.chromium.org/852463002/diff/60001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/852463002/diff/60001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:25: #include "content/browser/storage_partition_impl.h" On 2015/01/15 at 15:14:13, jochen (slow) wrote: > why the include? Used to be used. Good catch.
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6cdca919f433ffd0268b338fac478ef0cdea9949 Cr-Commit-Position: refs/heads/master@{#311681} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
