Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2556893003:
Remove ContextLifecycleObserver from NavigatorServiceWorker (Closed)
|
Created:
4 years ago by haraken Modified:
4 years ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, kinuko+serviceworker, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, tzik Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove ContextLifecycleObserver from NavigatorServiceWorker
NavigatorServiceWorker needs to reset m_serviceWorker when the context
gets destroyed. This CL makes ServiceWorkerContainer do the reset work
and removes ContextLifecycleObserver from NavigatorServiceWorker.
See the discussion on the CL for more rationale.
BUG=610176
Committed: https://crrev.com/b76d5e11df4955210c8634da71836f31762186c9
Cr-Commit-Position: refs/heads/master@{#438475}
Patch Set 1 #
Total comments: 1
Patch Set 2 : temp #
Total comments: 1
Patch Set 3 : temp #
Total comments: 3
Messages
Total messages: 39 (11 generated)
haraken@chromium.org changed reviewers: + nhiroki@chromium.org
PTAL
lgtm
The CQ bit was checked by haraken@chromium.org
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
service_worker_browsertest.cc is failing. I have a question. https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right): https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp:105: if (!m_serviceWorker && frame) { nhiroki@: Would you help me understand what this is doing? - It looks like this code is creating a new ServiceWorkerContainer after a frame associated with NavigatorServiceWorker gets detached. - Is the |frame| in this function equal to the frame associated with NavigatorServiceWorker? I think the answer is no, but it means that NavigatorServiceWorker is shared among multiple frames (over time). If so, we will need to clear m_serviceWorker every time the Navigator navigates away to a new frame. However, the current implementation clears m_serviceWorker only when the Navigator navigates away from the first frame (because ContextLifecycleObserver is registered only when NavigatorServiceWorker is constructed).
On 2016/12/12 02:47:51, haraken wrote: > service_worker_browsertest.cc is failing. I have a question. > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp > (right): > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp:105: > if (!m_serviceWorker && frame) { > > nhiroki@: Would you help me understand what this is doing? > > - It looks like this code is creating a new ServiceWorkerContainer after a frame > associated with NavigatorServiceWorker gets detached. > > - Is the |frame| in this function equal to the frame associated with > NavigatorServiceWorker? I think the answer is no, but it means that > NavigatorServiceWorker is shared among multiple frames (over time). If so, we > will need to clear m_serviceWorker every time the Navigator navigates away to a > new frame. However, the current implementation clears m_serviceWorker only when > the Navigator navigates away from the first frame (because > ContextLifecycleObserver is registered only when NavigatorServiceWorker is > constructed). I'm also getting confused about this behavior. Let me have a little time to investigate this...
On 2016/12/12 11:49:03, nhiroki wrote: > On 2016/12/12 02:47:51, haraken wrote: > > service_worker_browsertest.cc is failing. I have a question. > > > > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > > File > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp > > (right): > > > > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > > > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp:105: > > if (!m_serviceWorker && frame) { > > > > nhiroki@: Would you help me understand what this is doing? > > > > - It looks like this code is creating a new ServiceWorkerContainer after a > frame > > associated with NavigatorServiceWorker gets detached. > > > > - Is the |frame| in this function equal to the frame associated with > > NavigatorServiceWorker? I think the answer is no, but it means that > > NavigatorServiceWorker is shared among multiple frames (over time). If so, we > > will need to clear m_serviceWorker every time the Navigator navigates away to > a > > new frame. However, the current implementation clears m_serviceWorker only > when > > the Navigator navigates away from the first frame (because > > ContextLifecycleObserver is registered only when NavigatorServiceWorker is > > constructed). > > I'm also getting confused about this behavior. Let me have a little time to > investigate this... The current code looks surely confusing. For context, this class was originally using DOMWindow's willDetachGlobalObjectFromFrame() instead of Document's contextDestroyed, which was probably a little clearer about what it was doing. Reg: haraken's question: in my understanding (and from some quick testing) it should be the same frame. Navigator object doesn't seem supposed to be re-associated with a new frame, and window.navigator gets reset (to null) only when the frame gets a new DOMWindow. This code path could be taken when a frame continues to use the same DOMWindow while its document is reset, which could happen when the document transitions from about:blank or an initial empty page, and these seem to be the test failure cases. I'm not very familiar with the code around and I wasn't able to quickly find out other cases where m_serviceWorker could be re-associated to a new document other than for about:blank cases, but it feels the current code is a bit sketchy. The difference between the lifetime of this instance and document seems to be making the code confusing and possibly wrong. I think we could make this stop observing executionContext's contextLifecycle like this CL does but instead let ServiceWorkerContainer notify this class that this needs to reset m_serviceWorker in its contextDestroyed(), so we can make sure every time m_serviceWorker and its associated document destroyed this gets a new m_serviceWorker?
On 2016/12/13 03:17:59, kinuko wrote: > On 2016/12/12 11:49:03, nhiroki wrote: > > On 2016/12/12 02:47:51, haraken wrote: > > > service_worker_browsertest.cc is failing. I have a question. > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > > > File > > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > > > > > > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp:105: > > > if (!m_serviceWorker && frame) { > > > > > > nhiroki@: Would you help me understand what this is doing? > > > > > > - It looks like this code is creating a new ServiceWorkerContainer after a > > frame > > > associated with NavigatorServiceWorker gets detached. > > > > > > - Is the |frame| in this function equal to the frame associated with > > > NavigatorServiceWorker? I think the answer is no, but it means that > > > NavigatorServiceWorker is shared among multiple frames (over time). If so, > we > > > will need to clear m_serviceWorker every time the Navigator navigates away > to > > a > > > new frame. However, the current implementation clears m_serviceWorker only > > when > > > the Navigator navigates away from the first frame (because > > > ContextLifecycleObserver is registered only when NavigatorServiceWorker is > > > constructed). > > > > I'm also getting confused about this behavior. Let me have a little time to > > investigate this... > > The current code looks surely confusing. For context, this class was originally > using DOMWindow's willDetachGlobalObjectFromFrame() instead of Document's > contextDestroyed, which was probably a little clearer about what it was doing. > > Reg: haraken's question: in my understanding (and from some quick testing) it > should be the same frame. Navigator object doesn't seem supposed to be > re-associated with a new frame, and window.navigator gets reset (to null) only > when the frame gets a new DOMWindow. This code path could be taken when a frame > continues to use the same DOMWindow while its document is reset, which could > happen when the document transitions from about:blank or an initial empty page, I think this can also happen when a frame navigates away from one document to another document. > and these seem to be the test failure cases. I'm not very familiar with the > code around and I wasn't able to quickly find out other cases where > m_serviceWorker could be re-associated to a new document other than for > about:blank cases, but it feels the current code is a bit sketchy. > > The difference between the lifetime of this instance and document seems to be > making the code confusing and possibly wrong. I think we could make this stop > observing executionContext's contextLifecycle like this CL does but instead let > ServiceWorkerContainer notify this class that this needs to reset > m_serviceWorker in its contextDestroyed(), so we can make sure every time > m_serviceWorker and its associated document destroyed this gets a new > m_serviceWorker? If you need to recreate m_serviceWorker for each document, it won't be enough because contextDestroyed() is called only for the first document. The second or later frame navigation will be ignored. I'm just curious but what's a reason we cannot reuse m_serviceWorker over the frame navigations?
On 2016/12/13 05:38:21, haraken wrote: > On 2016/12/13 03:17:59, kinuko wrote: > > On 2016/12/12 11:49:03, nhiroki wrote: > > > On 2016/12/12 02:47:51, haraken wrote: > > > > service_worker_browsertest.cc is failing. I have a question. > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > > > > File > > > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > > > > > > > > > > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp:105: > > > > if (!m_serviceWorker && frame) { > > > > > > > > nhiroki@: Would you help me understand what this is doing? > > > > > > > > - It looks like this code is creating a new ServiceWorkerContainer after a > > > frame > > > > associated with NavigatorServiceWorker gets detached. > > > > > > > > - Is the |frame| in this function equal to the frame associated with > > > > NavigatorServiceWorker? I think the answer is no, but it means that > > > > NavigatorServiceWorker is shared among multiple frames (over time). If so, > > we > > > > will need to clear m_serviceWorker every time the Navigator navigates away > > to > > > a > > > > new frame. However, the current implementation clears m_serviceWorker only > > > when > > > > the Navigator navigates away from the first frame (because > > > > ContextLifecycleObserver is registered only when NavigatorServiceWorker is > > > > constructed). > > > > > > I'm also getting confused about this behavior. Let me have a little time to > > > investigate this... > > > > The current code looks surely confusing. For context, this class was > originally > > using DOMWindow's willDetachGlobalObjectFromFrame() instead of Document's > > contextDestroyed, which was probably a little clearer about what it was doing. > > > > Reg: haraken's question: in my understanding (and from some quick testing) it > > should be the same frame. Navigator object doesn't seem supposed to be > > re-associated with a new frame, and window.navigator gets reset (to null) only > > when the frame gets a new DOMWindow. This code path could be taken when a > frame > > continues to use the same DOMWindow while its document is reset, which could > > happen when the document transitions from about:blank or an initial empty > page, > > I think this can also happen when a frame navigates away from one document to > another document. > > > and these seem to be the test failure cases. I'm not very familiar with the > > code around and I wasn't able to quickly find out other cases where > > m_serviceWorker could be re-associated to a new document other than for > > about:blank cases, but it feels the current code is a bit sketchy. > > > > The difference between the lifetime of this instance and document seems to be > > making the code confusing and possibly wrong. I think we could make this stop > > observing executionContext's contextLifecycle like this CL does but instead > let > > ServiceWorkerContainer notify this class that this needs to reset > > m_serviceWorker in its contextDestroyed(), so we can make sure every time > > m_serviceWorker and its associated document destroyed this gets a new > > m_serviceWorker? > > If you need to recreate m_serviceWorker for each document, it won't be enough > because contextDestroyed() is called only for the first document. The second or > later frame navigation will be ignored. > > I'm just curious but what's a reason we cannot reuse m_serviceWorker over the > frame navigations? I think we have the same problem in WindowAnimationWorklet::m_animationWorklet. WindowAnimationWorklet::m_animationWorklet is recreated only when the frame navigates from the first document to the second document.
haraken@chromium.org changed reviewers: + dcheng@chromium.org
On 2016/12/13 05:38:21, haraken wrote: > On 2016/12/13 03:17:59, kinuko wrote: > > On 2016/12/12 11:49:03, nhiroki wrote: > > > On 2016/12/12 02:47:51, haraken wrote: > > > > service_worker_browsertest.cc is failing. I have a question. > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > > > > File > > > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > > > > > > > > > > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp:105: > > > > if (!m_serviceWorker && frame) { > > > > > > > > nhiroki@: Would you help me understand what this is doing? > > > > > > > > - It looks like this code is creating a new ServiceWorkerContainer after a > > > frame > > > > associated with NavigatorServiceWorker gets detached. > > > > > > > > - Is the |frame| in this function equal to the frame associated with > > > > NavigatorServiceWorker? I think the answer is no, but it means that > > > > NavigatorServiceWorker is shared among multiple frames (over time). If so, > > we > > > > will need to clear m_serviceWorker every time the Navigator navigates away > > to > > > a > > > > new frame. However, the current implementation clears m_serviceWorker only > > > when > > > > the Navigator navigates away from the first frame (because > > > > ContextLifecycleObserver is registered only when NavigatorServiceWorker is > > > > constructed). > > > > > > I'm also getting confused about this behavior. Let me have a little time to > > > investigate this... > > > > The current code looks surely confusing. For context, this class was > originally > > using DOMWindow's willDetachGlobalObjectFromFrame() instead of Document's > > contextDestroyed, which was probably a little clearer about what it was doing. > > > > Reg: haraken's question: in my understanding (and from some quick testing) it > > should be the same frame. Navigator object doesn't seem supposed to be > > re-associated with a new frame, and window.navigator gets reset (to null) only > > when the frame gets a new DOMWindow. This code path could be taken when a > frame > > continues to use the same DOMWindow while its document is reset, which could > > happen when the document transitions from about:blank or an initial empty > page, > > I think this can also happen when a frame navigates away from one document to > another document. I noticed that the above description is not correct. a) When a frame navigates away from a normal document to another document: - Frame is reused. - Window is recreated. - Navigator is recreated. - Document is recreated. b) When a frame navigates away from about:blank or an initial empty page to another document: - Frame is reused. - Window is reused. - Navigator is reused. - Document is recreated. And the problem we're talking here happens only in the case b). If m_serviceWorker is created in about:blank or an initial empty page, it needs to be recreated when the frame navigates to a new document (because the navigator is not recreated). Hmm. Then it looks valid to assume that the navigation happens only once. Then it might be correct to use ContextLifecycleObserver and clear m_serviceWorker so that m_serviceWorker is recreated after the navigation. That said, it looks confusing :/
On 2016/12/13 08:37:54, haraken wrote: > On 2016/12/13 05:38:21, haraken wrote: > > On 2016/12/13 03:17:59, kinuko wrote: > > > On 2016/12/12 11:49:03, nhiroki wrote: > > > > On 2016/12/12 02:47:51, haraken wrote: > > > > > service_worker_browsertest.cc is failing. I have a question. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > > > > > File > > > > > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > > > > > > > > > > > > > > > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp:105: > > > > > if (!m_serviceWorker && frame) { > > > > > > > > > > nhiroki@: Would you help me understand what this is doing? > > > > > > > > > > - It looks like this code is creating a new ServiceWorkerContainer after > a > > > > frame > > > > > associated with NavigatorServiceWorker gets detached. > > > > > > > > > > - Is the |frame| in this function equal to the frame associated with > > > > > NavigatorServiceWorker? I think the answer is no, but it means that > > > > > NavigatorServiceWorker is shared among multiple frames (over time). If > so, > > > we > > > > > will need to clear m_serviceWorker every time the Navigator navigates > away > > > to > > > > a > > > > > new frame. However, the current implementation clears m_serviceWorker > only > > > > when > > > > > the Navigator navigates away from the first frame (because > > > > > ContextLifecycleObserver is registered only when NavigatorServiceWorker > is > > > > > constructed). > > > > > > > > I'm also getting confused about this behavior. Let me have a little time > to > > > > investigate this... > > > > > > The current code looks surely confusing. For context, this class was > > originally > > > using DOMWindow's willDetachGlobalObjectFromFrame() instead of Document's > > > contextDestroyed, which was probably a little clearer about what it was > doing. > > > > > > Reg: haraken's question: in my understanding (and from some quick testing) > it > > > should be the same frame. Navigator object doesn't seem supposed to be > > > re-associated with a new frame, and window.navigator gets reset (to null) > only > > > when the frame gets a new DOMWindow. This code path could be taken when a > > frame > > > continues to use the same DOMWindow while its document is reset, which could > > > happen when the document transitions from about:blank or an initial empty > > page, > > > > I think this can also happen when a frame navigates away from one document to > > another document. > > I noticed that the above description is not correct. > > a) When a frame navigates away from a normal document to another document: > > - Frame is reused. > - Window is recreated. > - Navigator is recreated. > - Document is recreated. > > b) When a frame navigates away from about:blank or an initial empty page to > another document: > > - Frame is reused. > - Window is reused. > - Navigator is reused. > - Document is recreated. Just to clarify: this reuse *only* happens when navigating away from the initial empty document, and it *only* happens if the Document that will commit same-origin to the initla empty document. > > And the problem we're talking here happens only in the case b). If > m_serviceWorker is created in about:blank or an initial empty page, it needs to > be recreated when the frame navigates to a new document (because the navigator > is not recreated). > > Hmm. Then it looks valid to assume that the navigation happens only once. Then > it might be correct to use ContextLifecycleObserver and clear m_serviceWorker so > that m_serviceWorker is recreated after the navigation. That said, it looks > confusing :/ Yes, if we need to recreate it on each navigation, I think ContextLifecycleObserver is the right thing.
On 2016/12/13 08:56:46, dcheng wrote: > On 2016/12/13 08:37:54, haraken wrote: > > On 2016/12/13 05:38:21, haraken wrote: > > > On 2016/12/13 03:17:59, kinuko wrote: > > > > On 2016/12/12 11:49:03, nhiroki wrote: > > > > > On 2016/12/12 02:47:51, haraken wrote: > > > > > > service_worker_browsertest.cc is failing. I have a question. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > > > > > > File > > > > > > > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > > > > > > > > > > > > > > > > > > > > > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp:105: > > > > > > if (!m_serviceWorker && frame) { > > > > > > > > > > > > nhiroki@: Would you help me understand what this is doing? > > > > > > > > > > > > - It looks like this code is creating a new ServiceWorkerContainer > after > > a > > > > > frame > > > > > > associated with NavigatorServiceWorker gets detached. > > > > > > > > > > > > - Is the |frame| in this function equal to the frame associated with > > > > > > NavigatorServiceWorker? I think the answer is no, but it means that > > > > > > NavigatorServiceWorker is shared among multiple frames (over time). If > > so, > > > > we > > > > > > will need to clear m_serviceWorker every time the Navigator navigates > > away > > > > to > > > > > a > > > > > > new frame. However, the current implementation clears m_serviceWorker > > only > > > > > when > > > > > > the Navigator navigates away from the first frame (because > > > > > > ContextLifecycleObserver is registered only when > NavigatorServiceWorker > > is > > > > > > constructed). > > > > > > > > > > I'm also getting confused about this behavior. Let me have a little time > > to > > > > > investigate this... > > > > > > > > The current code looks surely confusing. For context, this class was > > > originally > > > > using DOMWindow's willDetachGlobalObjectFromFrame() instead of Document's > > > > contextDestroyed, which was probably a little clearer about what it was > > doing. > > > > > > > > Reg: haraken's question: in my understanding (and from some quick testing) > > it > > > > should be the same frame. Navigator object doesn't seem supposed to be > > > > re-associated with a new frame, and window.navigator gets reset (to null) > > only > > > > when the frame gets a new DOMWindow. This code path could be taken when a > > > frame > > > > continues to use the same DOMWindow while its document is reset, which > could > > > > happen when the document transitions from about:blank or an initial empty > > > page, > > > > > > I think this can also happen when a frame navigates away from one document > to > > > another document. > > > > I noticed that the above description is not correct. > > > > a) When a frame navigates away from a normal document to another document: > > > > - Frame is reused. > > - Window is recreated. > > - Navigator is recreated. > > - Document is recreated. > > > > b) When a frame navigates away from about:blank or an initial empty page to > > another document: > > > > - Frame is reused. > > - Window is reused. > > - Navigator is reused. > > - Document is recreated. > > Just to clarify: this reuse *only* happens when navigating away from the initial > empty document, and it *only* happens if the Document that will commit > same-origin to the initla empty document. Just to confirm: The navigation happens only once, right? Otherwise, it will be problematic because contextDestroyed() is called only once (unless you re-register to ContextLifecycleObserver). > > > > And the problem we're talking here happens only in the case b). If > > m_serviceWorker is created in about:blank or an initial empty page, it needs > to > > be recreated when the frame navigates to a new document (because the navigator > > is not recreated). > > > > Hmm. Then it looks valid to assume that the navigation happens only once. Then > > it might be correct to use ContextLifecycleObserver and clear m_serviceWorker > so > > that m_serviceWorker is recreated after the navigation. That said, it looks > > confusing :/ > > Yes, if we need to recreate it on each navigation, I think > ContextLifecycleObserver is the right thing. Yeah. However, my concern is that almost all objects do not care about the scenario. I want to understand why NavigatorServiceWorker is special. - Why do we need to recreate m_serviceWorker on each navigation? - Why is m_serviceWorker created in the initial empty page? (<--- Maybe is this the special aspect of NavigatorServiceworker?)
On 2016/12/13 09:01:52, haraken wrote: > On 2016/12/13 08:56:46, dcheng wrote: > > On 2016/12/13 08:37:54, haraken wrote: > > > On 2016/12/13 05:38:21, haraken wrote: > > > > On 2016/12/13 03:17:59, kinuko wrote: > > > > > On 2016/12/12 11:49:03, nhiroki wrote: > > > > > > On 2016/12/12 02:47:51, haraken wrote: > > > > > > > service_worker_browsertest.cc is failing. I have a question. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > > > > > > > File > > > > > > > > > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > > > > > > > > > > > > > > > > > > > > > > > > > > > > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp:105: > > > > > > > if (!m_serviceWorker && frame) { > > > > > > > > > > > > > > nhiroki@: Would you help me understand what this is doing? > > > > > > > > > > > > > > - It looks like this code is creating a new ServiceWorkerContainer > > after > > > a > > > > > > frame > > > > > > > associated with NavigatorServiceWorker gets detached. > > > > > > > > > > > > > > - Is the |frame| in this function equal to the frame associated with > > > > > > > NavigatorServiceWorker? I think the answer is no, but it means that > > > > > > > NavigatorServiceWorker is shared among multiple frames (over time). > If > > > so, > > > > > we > > > > > > > will need to clear m_serviceWorker every time the Navigator > navigates > > > away > > > > > to > > > > > > a > > > > > > > new frame. However, the current implementation clears > m_serviceWorker > > > only > > > > > > when > > > > > > > the Navigator navigates away from the first frame (because > > > > > > > ContextLifecycleObserver is registered only when > > NavigatorServiceWorker > > > is > > > > > > > constructed). > > > > > > > > > > > > I'm also getting confused about this behavior. Let me have a little > time > > > to > > > > > > investigate this... > > > > > > > > > > The current code looks surely confusing. For context, this class was > > > > originally > > > > > using DOMWindow's willDetachGlobalObjectFromFrame() instead of > Document's > > > > > contextDestroyed, which was probably a little clearer about what it was > > > doing. > > > > > > > > > > Reg: haraken's question: in my understanding (and from some quick > testing) > > > it > > > > > should be the same frame. Navigator object doesn't seem supposed to be > > > > > re-associated with a new frame, and window.navigator gets reset (to > null) > > > only > > > > > when the frame gets a new DOMWindow. This code path could be taken when > a > > > > frame > > > > > continues to use the same DOMWindow while its document is reset, which > > could > > > > > happen when the document transitions from about:blank or an initial > empty > > > > page, > > > > > > > > I think this can also happen when a frame navigates away from one document > > to > > > > another document. > > > > > > I noticed that the above description is not correct. > > > > > > a) When a frame navigates away from a normal document to another document: > > > > > > - Frame is reused. > > > - Window is recreated. > > > - Navigator is recreated. > > > - Document is recreated. > > > > > > b) When a frame navigates away from about:blank or an initial empty page to > > > another document: > > > > > > - Frame is reused. > > > - Window is reused. > > > - Navigator is reused. > > > - Document is recreated. > > > > Just to clarify: this reuse *only* happens when navigating away from the > initial > > empty document, and it *only* happens if the Document that will commit > > same-origin to the initla empty document. > > Just to confirm: The navigation happens only once, right? Otherwise, it will be > problematic because contextDestroyed() is called only once (unless you > re-register to ContextLifecycleObserver). I think I might not understand the question correctly but: - Initial empty page -> cross-origin document: we create a new LocalDOMWindow and install a new Document for the navigation on the new window. - Initial empty page -> same-origin document: we do not create a new LocalDOMWindow and just install a new Document for navigation on the original window. - Any other navigation: we create a new LocalDOMWindow and install a new Document for the navigation on the new window. The logic for this is here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Do... And in all cases, there is only one navigation. > > > > > > > And the problem we're talking here happens only in the case b). If > > > m_serviceWorker is created in about:blank or an initial empty page, it needs > > to > > > be recreated when the frame navigates to a new document (because the > navigator > > > is not recreated). > > > > > > Hmm. Then it looks valid to assume that the navigation happens only once. > Then > > > it might be correct to use ContextLifecycleObserver and clear > m_serviceWorker > > so > > > that m_serviceWorker is recreated after the navigation. That said, it looks > > > confusing :/ > > > > Yes, if we need to recreate it on each navigation, I think > > ContextLifecycleObserver is the right thing. > > Yeah. However, my concern is that almost all objects do not care about the > scenario. I want to understand why NavigatorServiceWorker is special. > > - Why do we need to recreate m_serviceWorker on each navigation? > - Why is m_serviceWorker created in the initial empty page? (<--- Maybe is this > the special aspect of NavigatorServiceworker?) I'm not sure, but I think it's typical to create Document's supplements/observer for the initial empty document? Not creating it seems like it would be more unusual (since ultimately, the initial empty page is just another document)
On 2016/12/13 09:06:55, dcheng wrote: > On 2016/12/13 09:01:52, haraken wrote: > > On 2016/12/13 08:56:46, dcheng wrote: > > > On 2016/12/13 08:37:54, haraken wrote: > > > > On 2016/12/13 05:38:21, haraken wrote: > > > > > On 2016/12/13 03:17:59, kinuko wrote: > > > > > > On 2016/12/12 11:49:03, nhiroki wrote: > > > > > > > On 2016/12/12 02:47:51, haraken wrote: > > > > > > > > service_worker_browsertest.cc is failing. I have a question. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > > > > > > > > File > > > > > > > > > > > > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp > > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_party/WebKit/Source/m... > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp:105: > > > > > > > > if (!m_serviceWorker && frame) { > > > > > > > > > > > > > > > > nhiroki@: Would you help me understand what this is doing? > > > > > > > > > > > > > > > > - It looks like this code is creating a new ServiceWorkerContainer > > > after > > > > a > > > > > > > frame > > > > > > > > associated with NavigatorServiceWorker gets detached. > > > > > > > > > > > > > > > > - Is the |frame| in this function equal to the frame associated > with > > > > > > > > NavigatorServiceWorker? I think the answer is no, but it means > that > > > > > > > > NavigatorServiceWorker is shared among multiple frames (over > time). > > If > > > > so, > > > > > > we > > > > > > > > will need to clear m_serviceWorker every time the Navigator > > navigates > > > > away > > > > > > to > > > > > > > a > > > > > > > > new frame. However, the current implementation clears > > m_serviceWorker > > > > only > > > > > > > when > > > > > > > > the Navigator navigates away from the first frame (because > > > > > > > > ContextLifecycleObserver is registered only when > > > NavigatorServiceWorker > > > > is > > > > > > > > constructed). > > > > > > > > > > > > > > I'm also getting confused about this behavior. Let me have a little > > time > > > > to > > > > > > > investigate this... > > > > > > > > > > > > The current code looks surely confusing. For context, this class was > > > > > originally > > > > > > using DOMWindow's willDetachGlobalObjectFromFrame() instead of > > Document's > > > > > > contextDestroyed, which was probably a little clearer about what it > was > > > > doing. > > > > > > > > > > > > Reg: haraken's question: in my understanding (and from some quick > > testing) > > > > it > > > > > > should be the same frame. Navigator object doesn't seem supposed to > be > > > > > > re-associated with a new frame, and window.navigator gets reset (to > > null) > > > > only > > > > > > when the frame gets a new DOMWindow. This code path could be taken > when > > a > > > > > frame > > > > > > continues to use the same DOMWindow while its document is reset, which > > > could > > > > > > happen when the document transitions from about:blank or an initial > > empty > > > > > page, > > > > > > > > > > I think this can also happen when a frame navigates away from one > document > > > to > > > > > another document. > > > > > > > > I noticed that the above description is not correct. > > > > > > > > a) When a frame navigates away from a normal document to another document: > > > > > > > > - Frame is reused. > > > > - Window is recreated. > > > > - Navigator is recreated. > > > > - Document is recreated. > > > > > > > > b) When a frame navigates away from about:blank or an initial empty page > to > > > > another document: > > > > > > > > - Frame is reused. > > > > - Window is reused. > > > > - Navigator is reused. > > > > - Document is recreated. > > > > > > Just to clarify: this reuse *only* happens when navigating away from the > > initial > > > empty document, and it *only* happens if the Document that will commit > > > same-origin to the initla empty document. > > > > Just to confirm: The navigation happens only once, right? Otherwise, it will > be > > problematic because contextDestroyed() is called only once (unless you > > re-register to ContextLifecycleObserver). > > I think I might not understand the question correctly but: > - Initial empty page -> cross-origin document: we create a new LocalDOMWindow > and install a new Document for the navigation on the new window. > - Initial empty page -> same-origin document: we do not create a new > LocalDOMWindow and just install a new Document for navigation on the original > window. > - Any other navigation: we create a new LocalDOMWindow and install a new > Document for the navigation on the new window. > > The logic for this is here: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Do... > > And in all cases, there is only one navigation. > > > > > > > > > > > And the problem we're talking here happens only in the case b). If > > > > m_serviceWorker is created in about:blank or an initial empty page, it > needs > > > to > > > > be recreated when the frame navigates to a new document (because the > > navigator > > > > is not recreated). > > > > > > > > Hmm. Then it looks valid to assume that the navigation happens only once. > > Then > > > > it might be correct to use ContextLifecycleObserver and clear > > m_serviceWorker > > > so > > > > that m_serviceWorker is recreated after the navigation. That said, it > looks > > > > confusing :/ > > > > > > Yes, if we need to recreate it on each navigation, I think > > > ContextLifecycleObserver is the right thing. > > > > Yeah. However, my concern is that almost all objects do not care about the > > scenario. I want to understand why NavigatorServiceWorker is special. > > > > - Why do we need to recreate m_serviceWorker on each navigation? > > - Why is m_serviceWorker created in the initial empty page? (<--- Maybe is > this > > the special aspect of NavigatorServiceworker?) > > I'm not sure, but I think it's typical to create Document's supplements/observer > for the initial empty document? Not creating it seems like it would be more > unusual (since ultimately, the initial empty page is just another document) Yeah, it looks like that we create supplements for the initial empty document but don't recreate the supplements when navigating from a same-origin initial empty page: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Do... So my question will boil down to: - Why do we need to create m_serviceWorker when navigating from a same-origin initial empty page? That's why m_serviceWorker needs the special treatment using ContextLifecycleObserver.
On Tue, Dec 13, 2016 at 2:38 PM, <haraken@chromium.org> wrote: > On 2016/12/13 03:17:59, kinuko wrote: > > On 2016/12/12 11:49:03, nhiroki wrote: > > > On 2016/12/12 02:47:51, haraken wrote: > > > > service_worker_browsertest.cc is failing. I have a question. > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_ > party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp > > > > File > > > third_party/WebKit/Source/modules/serviceworkers/ > NavigatorServiceWorker.cpp > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_ > party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp# > newcode105 > > > > > > > > > > third_party/WebKit/Source/modules/serviceworkers/ > NavigatorServiceWorker.cpp:105: > > > > if (!m_serviceWorker && frame) { > > > > > > > > nhiroki@: Would you help me understand what this is doing? > > > > > > > > - It looks like this code is creating a new ServiceWorkerContainer > after a > > > frame > > > > associated with NavigatorServiceWorker gets detached. > > > > > > > > - Is the |frame| in this function equal to the frame associated with > > > > NavigatorServiceWorker? I think the answer is no, but it means that > > > > NavigatorServiceWorker is shared among multiple frames (over time). > If so, > > we > > > > will need to clear m_serviceWorker every time the Navigator > navigates away > > to > > > a > > > > new frame. However, the current implementation clears > m_serviceWorker only > > > when > > > > the Navigator navigates away from the first frame (because > > > > ContextLifecycleObserver is registered only when > NavigatorServiceWorker is > > > > constructed). > > > > > > I'm also getting confused about this behavior. Let me have a little > time to > > > investigate this... > > > > The current code looks surely confusing. For context, this class was > originally > > using DOMWindow's willDetachGlobalObjectFromFrame() instead of > Document's > > contextDestroyed, which was probably a little clearer about what it was > doing. > > > > Reg: haraken's question: in my understanding (and from some quick > testing) it > > should be the same frame. Navigator object doesn't seem supposed to be > > re-associated with a new frame, and window.navigator gets reset (to > null) only > > when the frame gets a new DOMWindow. This code path could be taken when a > frame > > continues to use the same DOMWindow while its document is reset, which > could > > happen when the document transitions from about:blank or an initial empty > page, > > I think this can also happen when a frame navigates away from one document > to > another document. > > > and these seem to be the test failure cases. I'm not very familiar with > the > > code around and I wasn't able to quickly find out other cases where > > m_serviceWorker could be re-associated to a new document other than for > > about:blank cases, but it feels the current code is a bit sketchy. > > > > The difference between the lifetime of this instance and document seems > to be > > making the code confusing and possibly wrong. I think we could make this > stop > > observing executionContext's contextLifecycle like this CL does but > instead > let > > ServiceWorkerContainer notify this class that this needs to reset > > m_serviceWorker in its contextDestroyed(), so we can make sure every time > > m_serviceWorker and its associated document destroyed this gets a new > > m_serviceWorker? > > If you need to recreate m_serviceWorker for each document, it won't be > enough > because contextDestroyed() is called only for the first document. The > second or > later frame navigation will be ignored. > What I meant to say is to let ServiceWorkerContainer have a back pointer (say, as a WeakMember) to NavigatorServiceWorker and let it reset NavigatorServiceWorker's m_serviceWorker when ServiceWorkerContainer's contextDestroyed() is called. Since ServiceWorkerContainer is recreated as a lifecycleObserver of each new document it seems to work even if we have second or later documents. Regardless of whether it could happen or not it feels it's less confusing to me. > I'm just curious but what's a reason we cannot reuse m_serviceWorker over > the > frame navigations? > m_serviceWorker represents a client document for the service worker that is controlling the document and it needs to be tied to a document. It's making the situation slightly complicated as m_serviceWorker needs to be accessible via window.navigator which could have a different lifetime from the document as we've just seen. https://codereview.chromium.org/2556893003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Tue, Dec 13, 2016 at 2:38 PM, <haraken@chromium.org> wrote: > On 2016/12/13 03:17:59, kinuko wrote: > > On 2016/12/12 11:49:03, nhiroki wrote: > > > On 2016/12/12 02:47:51, haraken wrote: > > > > service_worker_browsertest.cc is failing. I have a question. > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_ > party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp > > > > File > > > third_party/WebKit/Source/modules/serviceworkers/ > NavigatorServiceWorker.cpp > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_ > party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp# > newcode105 > > > > > > > > > > third_party/WebKit/Source/modules/serviceworkers/ > NavigatorServiceWorker.cpp:105: > > > > if (!m_serviceWorker && frame) { > > > > > > > > nhiroki@: Would you help me understand what this is doing? > > > > > > > > - It looks like this code is creating a new ServiceWorkerContainer > after a > > > frame > > > > associated with NavigatorServiceWorker gets detached. > > > > > > > > - Is the |frame| in this function equal to the frame associated with > > > > NavigatorServiceWorker? I think the answer is no, but it means that > > > > NavigatorServiceWorker is shared among multiple frames (over time). > If so, > > we > > > > will need to clear m_serviceWorker every time the Navigator > navigates away > > to > > > a > > > > new frame. However, the current implementation clears > m_serviceWorker only > > > when > > > > the Navigator navigates away from the first frame (because > > > > ContextLifecycleObserver is registered only when > NavigatorServiceWorker is > > > > constructed). > > > > > > I'm also getting confused about this behavior. Let me have a little > time to > > > investigate this... > > > > The current code looks surely confusing. For context, this class was > originally > > using DOMWindow's willDetachGlobalObjectFromFrame() instead of > Document's > > contextDestroyed, which was probably a little clearer about what it was > doing. > > > > Reg: haraken's question: in my understanding (and from some quick > testing) it > > should be the same frame. Navigator object doesn't seem supposed to be > > re-associated with a new frame, and window.navigator gets reset (to > null) only > > when the frame gets a new DOMWindow. This code path could be taken when a > frame > > continues to use the same DOMWindow while its document is reset, which > could > > happen when the document transitions from about:blank or an initial empty > page, > > I think this can also happen when a frame navigates away from one document > to > another document. > > > and these seem to be the test failure cases. I'm not very familiar with > the > > code around and I wasn't able to quickly find out other cases where > > m_serviceWorker could be re-associated to a new document other than for > > about:blank cases, but it feels the current code is a bit sketchy. > > > > The difference between the lifetime of this instance and document seems > to be > > making the code confusing and possibly wrong. I think we could make this > stop > > observing executionContext's contextLifecycle like this CL does but > instead > let > > ServiceWorkerContainer notify this class that this needs to reset > > m_serviceWorker in its contextDestroyed(), so we can make sure every time > > m_serviceWorker and its associated document destroyed this gets a new > > m_serviceWorker? > > If you need to recreate m_serviceWorker for each document, it won't be > enough > because contextDestroyed() is called only for the first document. The > second or > later frame navigation will be ignored. > What I meant to say is to let ServiceWorkerContainer have a back pointer (say, as a WeakMember) to NavigatorServiceWorker and let it reset NavigatorServiceWorker's m_serviceWorker when ServiceWorkerContainer's contextDestroyed() is called. Since ServiceWorkerContainer is recreated as a lifecycleObserver of each new document it seems to work even if we have second or later documents. Regardless of whether it could happen or not it feels it's less confusing to me. > I'm just curious but what's a reason we cannot reuse m_serviceWorker over > the > frame navigations? > m_serviceWorker represents a client document for the service worker that is controlling the document and it needs to be tied to a document. It's making the situation slightly complicated as m_serviceWorker needs to be accessible via window.navigator which could have a different lifetime from the document as we've just seen. https://codereview.chromium.org/2556893003/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
By the way we create NavigatorServiceWorker not in installSupplements but in FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld(), which could also originate from DocumentLoader::createWriterFor. This is not called when a frame navigates from an empty page either as DOMWindow is reused in the case. On Tue, Dec 13, 2016 at 6:12 PM, <haraken@chromium.org> wrote: > On 2016/12/13 09:06:55, dcheng wrote: > > On 2016/12/13 09:01:52, haraken wrote: > > > On 2016/12/13 08:56:46, dcheng wrote: > > > > On 2016/12/13 08:37:54, haraken wrote: > > > > > On 2016/12/13 05:38:21, haraken wrote: > > > > > > On 2016/12/13 03:17:59, kinuko wrote: > > > > > > > On 2016/12/12 11:49:03, nhiroki wrote: > > > > > > > > On 2016/12/12 02:47:51, haraken wrote: > > > > > > > > > service_worker_browsertest.cc is failing. I have a > question. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_ > party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp > > > > > > > > > File > > > > > > > > > > > > > > > third_party/WebKit/Source/modules/serviceworkers/ > NavigatorServiceWorker.cpp > > > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_ > party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp# > newcode105 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > third_party/WebKit/Source/modules/serviceworkers/ > NavigatorServiceWorker.cpp:105: > > > > > > > > > if (!m_serviceWorker && frame) { > > > > > > > > > > > > > > > > > > nhiroki@: Would you help me understand what this is doing? > > > > > > > > > > > > > > > > > > - It looks like this code is creating a new > ServiceWorkerContainer > > > > after > > > > > a > > > > > > > > frame > > > > > > > > > associated with NavigatorServiceWorker gets detached. > > > > > > > > > > > > > > > > > > - Is the |frame| in this function equal to the frame > associated > > with > > > > > > > > > NavigatorServiceWorker? I think the answer is no, but it > means > > that > > > > > > > > > NavigatorServiceWorker is shared among multiple frames > (over > > time). > > > If > > > > > so, > > > > > > > we > > > > > > > > > will need to clear m_serviceWorker every time the Navigator > > > navigates > > > > > away > > > > > > > to > > > > > > > > a > > > > > > > > > new frame. However, the current implementation clears > > > m_serviceWorker > > > > > only > > > > > > > > when > > > > > > > > > the Navigator navigates away from the first frame (because > > > > > > > > > ContextLifecycleObserver is registered only when > > > > NavigatorServiceWorker > > > > > is > > > > > > > > > constructed). > > > > > > > > > > > > > > > > I'm also getting confused about this behavior. Let me have a > little > > > time > > > > > to > > > > > > > > investigate this... > > > > > > > > > > > > > > The current code looks surely confusing. For context, this > class > was > > > > > > originally > > > > > > > using DOMWindow's willDetachGlobalObjectFromFrame() instead of > > > Document's > > > > > > > contextDestroyed, which was probably a little clearer about > what it > > was > > > > > doing. > > > > > > > > > > > > > > Reg: haraken's question: in my understanding (and from some > quick > > > testing) > > > > > it > > > > > > > should be the same frame. Navigator object doesn't seem > supposed to > > be > > > > > > > re-associated with a new frame, and window.navigator gets > reset (to > > > null) > > > > > only > > > > > > > when the frame gets a new DOMWindow. This code path could be > taken > > when > > > a > > > > > > frame > > > > > > > continues to use the same DOMWindow while its document is > reset, > which > > > > could > > > > > > > happen when the document transitions from about:blank or an > initial > > > empty > > > > > > page, > > > > > > > > > > > > I think this can also happen when a frame navigates away from one > > document > > > > to > > > > > > another document. > > > > > > > > > > I noticed that the above description is not correct. > > > > > > > > > > a) When a frame navigates away from a normal document to another > document: > > > > > > > > > > - Frame is reused. > > > > > - Window is recreated. > > > > > - Navigator is recreated. > > > > > - Document is recreated. > > > > > > > > > > b) When a frame navigates away from about:blank or an initial > empty page > > to > > > > > another document: > > > > > > > > > > - Frame is reused. > > > > > - Window is reused. > > > > > - Navigator is reused. > > > > > - Document is recreated. > > > > > > > > Just to clarify: this reuse *only* happens when navigating away from > the > > > initial > > > > empty document, and it *only* happens if the Document that will > commit > > > > same-origin to the initla empty document. > > > > > > Just to confirm: The navigation happens only once, right? Otherwise, > it will > > be > > > problematic because contextDestroyed() is called only once (unless you > > > re-register to ContextLifecycleObserver). > > > > I think I might not understand the question correctly but: > > - Initial empty page -> cross-origin document: we create a new > LocalDOMWindow > > and install a new Document for the navigation on the new window. > > - Initial empty page -> same-origin document: we do not create a new > > LocalDOMWindow and just install a new Document for navigation on the > original > > window. > > - Any other navigation: we create a new LocalDOMWindow and install a new > > Document for the navigation on the new window. > > > > The logic for this is here: > > > https://cs.chromium.org/chromium/src/third_party/ > WebKit/Source/core/loader/DocumentLoader.cpp?rcl=0&l=768 > > > > And in all cases, there is only one navigation. > > > > > > > > > > > > > > > And the problem we're talking here happens only in the case b). If > > > > > m_serviceWorker is created in about:blank or an initial empty > page, it > > needs > > > > to > > > > > be recreated when the frame navigates to a new document (because > the > > > navigator > > > > > is not recreated). > > > > > > > > > > Hmm. Then it looks valid to assume that the navigation happens only > once. > > > Then > > > > > it might be correct to use ContextLifecycleObserver and clear > > > m_serviceWorker > > > > so > > > > > that m_serviceWorker is recreated after the navigation. That said, > it > > looks > > > > > confusing :/ > > > > > > > > Yes, if we need to recreate it on each navigation, I think > > > > ContextLifecycleObserver is the right thing. > > > > > > Yeah. However, my concern is that almost all objects do not care about > the > > > scenario. I want to understand why NavigatorServiceWorker is special. > > > > > > - Why do we need to recreate m_serviceWorker on each navigation? > > > - Why is m_serviceWorker created in the initial empty page? (<--- > Maybe is > > this > > > the special aspect of NavigatorServiceworker?) > > > > I'm not sure, but I think it's typical to create Document's > supplements/observer > > for the initial empty document? Not creating it seems like it would be > more > > unusual (since ultimately, the initial empty page is just another > document) > > Yeah, it looks like that we create supplements for the initial empty > document > but don't recreate the supplements when navigating from a same-origin > initial > empty page: > > https://cs.chromium.org/chromium/src/third_party/ > WebKit/Source/core/loader/DocumentLoader.cpp?sq=package: > chromium&dr&rcl=1481602860&l=774 > > So my question will boil down to: > > - Why do we need to create m_serviceWorker when navigating from a > same-origin > initial empty page? > > That's why m_serviceWorker needs the special treatment using > ContextLifecycleObserver. > > > https://codereview.chromium.org/2556893003/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
By the way we create NavigatorServiceWorker not in installSupplements but in FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld(), which could also originate from DocumentLoader::createWriterFor. This is not called when a frame navigates from an empty page either as DOMWindow is reused in the case. On Tue, Dec 13, 2016 at 6:12 PM, <haraken@chromium.org> wrote: > On 2016/12/13 09:06:55, dcheng wrote: > > On 2016/12/13 09:01:52, haraken wrote: > > > On 2016/12/13 08:56:46, dcheng wrote: > > > > On 2016/12/13 08:37:54, haraken wrote: > > > > > On 2016/12/13 05:38:21, haraken wrote: > > > > > > On 2016/12/13 03:17:59, kinuko wrote: > > > > > > > On 2016/12/12 11:49:03, nhiroki wrote: > > > > > > > > On 2016/12/12 02:47:51, haraken wrote: > > > > > > > > > service_worker_browsertest.cc is failing. I have a > question. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_ > party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp > > > > > > > > > File > > > > > > > > > > > > > > > third_party/WebKit/Source/modules/serviceworkers/ > NavigatorServiceWorker.cpp > > > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2556893003/diff/1/third_ > party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp# > newcode105 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > third_party/WebKit/Source/modules/serviceworkers/ > NavigatorServiceWorker.cpp:105: > > > > > > > > > if (!m_serviceWorker && frame) { > > > > > > > > > > > > > > > > > > nhiroki@: Would you help me understand what this is doing? > > > > > > > > > > > > > > > > > > - It looks like this code is creating a new > ServiceWorkerContainer > > > > after > > > > > a > > > > > > > > frame > > > > > > > > > associated with NavigatorServiceWorker gets detached. > > > > > > > > > > > > > > > > > > - Is the |frame| in this function equal to the frame > associated > > with > > > > > > > > > NavigatorServiceWorker? I think the answer is no, but it > means > > that > > > > > > > > > NavigatorServiceWorker is shared among multiple frames > (over > > time). > > > If > > > > > so, > > > > > > > we > > > > > > > > > will need to clear m_serviceWorker every time the Navigator > > > navigates > > > > > away > > > > > > > to > > > > > > > > a > > > > > > > > > new frame. However, the current implementation clears > > > m_serviceWorker > > > > > only > > > > > > > > when > > > > > > > > > the Navigator navigates away from the first frame (because > > > > > > > > > ContextLifecycleObserver is registered only when > > > > NavigatorServiceWorker > > > > > is > > > > > > > > > constructed). > > > > > > > > > > > > > > > > I'm also getting confused about this behavior. Let me have a > little > > > time > > > > > to > > > > > > > > investigate this... > > > > > > > > > > > > > > The current code looks surely confusing. For context, this > class > was > > > > > > originally > > > > > > > using DOMWindow's willDetachGlobalObjectFromFrame() instead of > > > Document's > > > > > > > contextDestroyed, which was probably a little clearer about > what it > > was > > > > > doing. > > > > > > > > > > > > > > Reg: haraken's question: in my understanding (and from some > quick > > > testing) > > > > > it > > > > > > > should be the same frame. Navigator object doesn't seem > supposed to > > be > > > > > > > re-associated with a new frame, and window.navigator gets > reset (to > > > null) > > > > > only > > > > > > > when the frame gets a new DOMWindow. This code path could be > taken > > when > > > a > > > > > > frame > > > > > > > continues to use the same DOMWindow while its document is > reset, > which > > > > could > > > > > > > happen when the document transitions from about:blank or an > initial > > > empty > > > > > > page, > > > > > > > > > > > > I think this can also happen when a frame navigates away from one > > document > > > > to > > > > > > another document. > > > > > > > > > > I noticed that the above description is not correct. > > > > > > > > > > a) When a frame navigates away from a normal document to another > document: > > > > > > > > > > - Frame is reused. > > > > > - Window is recreated. > > > > > - Navigator is recreated. > > > > > - Document is recreated. > > > > > > > > > > b) When a frame navigates away from about:blank or an initial > empty page > > to > > > > > another document: > > > > > > > > > > - Frame is reused. > > > > > - Window is reused. > > > > > - Navigator is reused. > > > > > - Document is recreated. > > > > > > > > Just to clarify: this reuse *only* happens when navigating away from > the > > > initial > > > > empty document, and it *only* happens if the Document that will > commit > > > > same-origin to the initla empty document. > > > > > > Just to confirm: The navigation happens only once, right? Otherwise, > it will > > be > > > problematic because contextDestroyed() is called only once (unless you > > > re-register to ContextLifecycleObserver). > > > > I think I might not understand the question correctly but: > > - Initial empty page -> cross-origin document: we create a new > LocalDOMWindow > > and install a new Document for the navigation on the new window. > > - Initial empty page -> same-origin document: we do not create a new > > LocalDOMWindow and just install a new Document for navigation on the > original > > window. > > - Any other navigation: we create a new LocalDOMWindow and install a new > > Document for the navigation on the new window. > > > > The logic for this is here: > > > https://cs.chromium.org/chromium/src/third_party/ > WebKit/Source/core/loader/DocumentLoader.cpp?rcl=0&l=768 > > > > And in all cases, there is only one navigation. > > > > > > > > > > > > > > > And the problem we're talking here happens only in the case b). If > > > > > m_serviceWorker is created in about:blank or an initial empty > page, it > > needs > > > > to > > > > > be recreated when the frame navigates to a new document (because > the > > > navigator > > > > > is not recreated). > > > > > > > > > > Hmm. Then it looks valid to assume that the navigation happens only > once. > > > Then > > > > > it might be correct to use ContextLifecycleObserver and clear > > > m_serviceWorker > > > > so > > > > > that m_serviceWorker is recreated after the navigation. That said, > it > > looks > > > > > confusing :/ > > > > > > > > Yes, if we need to recreate it on each navigation, I think > > > > ContextLifecycleObserver is the right thing. > > > > > > Yeah. However, my concern is that almost all objects do not care about > the > > > scenario. I want to understand why NavigatorServiceWorker is special. > > > > > > - Why do we need to recreate m_serviceWorker on each navigation? > > > - Why is m_serviceWorker created in the initial empty page? (<--- > Maybe is > > this > > > the special aspect of NavigatorServiceworker?) > > > > I'm not sure, but I think it's typical to create Document's > supplements/observer > > for the initial empty document? Not creating it seems like it would be > more > > unusual (since ultimately, the initial empty page is just another > document) > > Yeah, it looks like that we create supplements for the initial empty > document > but don't recreate the supplements when navigating from a same-origin > initial > empty page: > > https://cs.chromium.org/chromium/src/third_party/ > WebKit/Source/core/loader/DocumentLoader.cpp?sq=package: > chromium&dr&rcl=1481602860&l=774 > > So my question will boil down to: > > - Why do we need to create m_serviceWorker when navigating from a > same-origin > initial empty page? > > That's why m_serviceWorker needs the special treatment using > ContextLifecycleObserver. > > > https://codereview.chromium.org/2556893003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
(Meh. Above messages are from kinuko@. Not sure why they're duplicated, sorry if I spammed you :()
Description was changed from ========== Remove ContextLifecycleObserver from NavigatorServiceWorker I don't see any reason we have to clear m_serviceWorker in NavigatorServiceWorker::contextDestroyed(). This CL removes the logic and stop inheriting from ContextLifecycleObserver. BUG=610176 ========== to ========== Remove ContextLifecycleObserver from NavigatorServiceWorker NavigatorServiceWorker needs to reset m_serviceWorker when the context gets destroyed. This CL makes ServiceWorkerContainer do the reset work and removes ContextLifecycleObserver from NavigatorServiceWorker. See the discussion on the CL for more rationale. BUG=610176 ==========
Thanks kinuko-san! Updated the CL as you suggested. PTAL.
Thanks! lgtm https://codereview.chromium.org/2556893003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.h (right): https://codereview.chromium.org/2556893003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.h:107: explicit ServiceWorkerContainer(ExecutionContext*, NavigatorServiceWorker*); nit: no need of explicit now
> https://codereview.chromium.org/2556893003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.h > (right): > > https://codereview.chromium.org/2556893003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.h:107: > explicit ServiceWorkerContainer(ExecutionContext*, NavigatorServiceWorker*); > nit: no need of explicit now Done.
The CQ bit was checked by haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2556893003/#ps40001 (title: "temp")
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": 40001, "attempt_start_ts": 1481702609436270,
"parent_rev": "e3948b9854ad384ff4e726b626903845fabc0f37", "commit_rev":
"de9c888592270bdfc41376ce3f0242750c7e6803"}
Message was sent while issue was closed.
Description was changed from ========== Remove ContextLifecycleObserver from NavigatorServiceWorker NavigatorServiceWorker needs to reset m_serviceWorker when the context gets destroyed. This CL makes ServiceWorkerContainer do the reset work and removes ContextLifecycleObserver from NavigatorServiceWorker. See the discussion on the CL for more rationale. BUG=610176 ========== to ========== Remove ContextLifecycleObserver from NavigatorServiceWorker NavigatorServiceWorker needs to reset m_serviceWorker when the context gets destroyed. This CL makes ServiceWorkerContainer do the reset work and removes ContextLifecycleObserver from NavigatorServiceWorker. See the discussion on the CL for more rationale. BUG=610176 Review-Url: https://codereview.chromium.org/2556893003 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Remove ContextLifecycleObserver from NavigatorServiceWorker NavigatorServiceWorker needs to reset m_serviceWorker when the context gets destroyed. This CL makes ServiceWorkerContainer do the reset work and removes ContextLifecycleObserver from NavigatorServiceWorker. See the discussion on the CL for more rationale. BUG=610176 Review-Url: https://codereview.chromium.org/2556893003 ========== to ========== Remove ContextLifecycleObserver from NavigatorServiceWorker NavigatorServiceWorker needs to reset m_serviceWorker when the context gets destroyed. This CL makes ServiceWorkerContainer do the reset work and removes ContextLifecycleObserver from NavigatorServiceWorker. See the discussion on the CL for more rationale. BUG=610176 Committed: https://crrev.com/b76d5e11df4955210c8634da71836f31762186c9 Cr-Commit-Position: refs/heads/master@{#438475} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b76d5e11df4955210c8634da71836f31762186c9 Cr-Commit-Position: refs/heads/master@{#438475}
Message was sent while issue was closed.
wez@chromium.org changed reviewers: + wez@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2556893003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): https://codereview.chromium.org/2556893003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.cpp:141: if (m_navigator) Why check m_navigator here? It doesn't look like it is valid for it to be nullptr.
Message was sent while issue was closed.
https://codereview.chromium.org/2556893003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): https://codereview.chromium.org/2556893003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.cpp:141: if (m_navigator) On 2016/12/15 17:57:43, Wez wrote: > Why check m_navigator here? It doesn't look like it is valid for it to be > nullptr. It happens only in testing. I can pass in a dummy Navigator if you prefer it.
Message was sent while issue was closed.
https://codereview.chromium.org/2556893003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): https://codereview.chromium.org/2556893003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.cpp:141: if (m_navigator) On 2016/12/16 01:08:31, haraken wrote: > On 2016/12/15 17:57:43, Wez wrote: > > Why check m_navigator here? It doesn't look like it is valid for it to be > > nullptr. > > It happens only in testing. I can pass in a dummy Navigator if you prefer it. > That would be my suggestion, since it leads to a simpler API contract (passing a null Navigator is definitely an error), but this was a drive-by comment so feeel free to ignore. :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
