Install supplements at every navigation

Install supplements at every navigation

Currently supplement objects are reused when an initial empty document navigates to
a same-origin document. This is problematic because ContextLifecycleObservers are
detached when the initial empty document is destroyed. They cannot be used after
the navigation.

To fix the problem, we need to install supplements at every navigation.

BUG=610176
Committed: https://crrev.com/dd2e0942b294948ee891b1e54ca17a195fc1f3a1
Cr-Commit-Position: refs/heads/master@{#439413}

[haraken, 2016-12-16 08:03:25]

The CQ bit was checked by haraken@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 08:03:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 09:48:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 09:48:42]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-12-16 10:14:00]

Description was changed from

==========
Don't install supplements on the initial empty document

BUG=610176
==========

to

==========
Don't install supplements on the initial empty document

Currently we install supplement objects on the initial empty document.          
                       
This is problematic for supplement objects that inherit from                    
                    
ContextLifecycleObserver:                                                       
                    

1) The frame displays the initial empty document.
2) The frame navigates to a new same-origin document. At this point,            
                    
   supplement objects that inherit from ContextLifecycleObserver receive        
                    
   contextDestroyed and stop working. The frame is reused and thus              
                    
   the supplement objects are also reused.                                      
                    
3) The new document tries to access the supplement objects and fails.
                                                                                
                    
This CL prevents the scenario by not installing supplement                      
         
objects to the initial empty document.

BUG=610176
==========

[haraken, 2016-12-16 10:14:00]

haraken@chromium.org changed reviewers:
+ dcheng@chromium.org, sigbjornf@opera.com

[haraken, 2016-12-16 10:14:38]

dcheng@: PTAL

I confirmed that the CL passes all tests but I'm not sure if you like the
approach.

[haraken, 2016-12-16 10:15:14]

Background: This CL is needed to replace DOMWindowProperty on
ScreenOrientationControllerImpl with ContextLifecycleObserver.

[sof, 2016-12-16 10:57:52]

https://codereview.chromium.org/2580753004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2580753004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:797: if
(!frame->loader().stateMachine()->isDisplayingInitialEmptyDocument())
I need to think about this some, but don't you need to consider
!init.shouldReuseDefaultView() also/still?

[haraken, 2016-12-16 11:06:46]

https://codereview.chromium.org/2580753004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2580753004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:797: if
(!frame->loader().stateMachine()->isDisplayingInitialEmptyDocument())
On 2016/12/16 10:57:52, sof wrote:
> I need to think about this some, but don't you need to consider
> !init.shouldReuseDefaultView() also/still?

Unfortunately we cannot use it.

init.shouldReuseDefaultView() returns if the new document (after the navigation)
should reuse the window. However, what we want to check here is if the current
document can be reused in the next navigation.

[sof, 2016-12-16 15:54:33]

https://codereview.chromium.org/2580753004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2580753004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:797: if
(!frame->loader().stateMachine()->isDisplayingInitialEmptyDocument())
On 2016/12/16 11:06:46, haraken wrote:
> On 2016/12/16 10:57:52, sof wrote:
> > I need to think about this some, but don't you need to consider
> > !init.shouldReuseDefaultView() also/still?
> 
> Unfortunately we cannot use it.
> 
> init.shouldReuseDefaultView() returns if the new document (after the
navigation)
> should reuse the window. However, what we want to check here is if the current
> document can be reused in the next navigation.

ok, I see now - if the context is "empty doc and cross-origin" you do also want
to delay creating the supplements, just like you would for the same-origin case.

The logic isn't the easiest to grasp, but correct.

[dcheng, 2016-12-16 18:13:13]

Isn't the root cause that we're calling contextDestroyed() when it's not really
destroyed? It does seem like the initial empty document should still have
supplements.

[sof, 2016-12-16 18:42:33]

On 2016/12/16 18:13:13, dcheng wrote:
> Isn't the root cause that we're calling contextDestroyed() when it's not
really
> destroyed? It does seem like the initial empty document should still have
> supplements.

(Empty) document shutdown/detach will trigger the detach from the frame (which
clears the supplements) in FrameLoader::prepareForCommit() -
https://codereview.chromium.org/2568913003/#msg27 has details.

[haraken, 2016-12-17 02:07:56]

On 2016/12/16 18:13:13, dcheng wrote:
> Isn't the root cause that we're calling contextDestroyed() when it's not
really
> destroyed? It does seem like the initial empty document should still have
> supplements.

We're calling contextDestroyed because the initial empty document is actually
destroyed :) Note that the frame is reused but the document is destroyed.

One solution is to keep both LocalFrameLifecycleObserver and
ContextLifecycleObserver and ask people to use LocalFrameLifecycleObserver for a
per-frame thing. However, the distinction is very subtle and already broken in
many places. So I think it would be nicer if we can unify them into
ContextLifecycleObserver.

[dcheng, 2016-12-17 20:25:43]

On 2016/12/17 02:07:56, haraken wrote:
> On 2016/12/16 18:13:13, dcheng wrote:
> > Isn't the root cause that we're calling contextDestroyed() when it's not
> really
> > destroyed? It does seem like the initial empty document should still have
> > supplements.
> 
> We're calling contextDestroyed because the initial empty document is actually
> destroyed :) Note that the frame is reused but the document is destroyed.
> 
> One solution is to keep both LocalFrameLifecycleObserver and
> ContextLifecycleObserver and ask people to use LocalFrameLifecycleObserver for
a
> per-frame thing. However, the distinction is very subtle and already broken in
> many places. So I think it would be nicer if we can unify them into
> ContextLifecycleObserver.

Doesn't this mean an initial empty document wouldn't be able to use web APIs
that are supplied by supplements, such as VR, NFC, beacon, bluetooth, etc?

[haraken, 2016-12-18 13:34:59]

On 2016/12/17 20:25:43, dcheng wrote:
> On 2016/12/17 02:07:56, haraken wrote:
> > On 2016/12/16 18:13:13, dcheng wrote:
> > > Isn't the root cause that we're calling contextDestroyed() when it's not
> > really
> > > destroyed? It does seem like the initial empty document should still have
> > > supplements.
> > 
> > We're calling contextDestroyed because the initial empty document is
actually
> > destroyed :) Note that the frame is reused but the document is destroyed.
> > 
> > One solution is to keep both LocalFrameLifecycleObserver and
> > ContextLifecycleObserver and ask people to use LocalFrameLifecycleObserver
for
> a
> > per-frame thing. However, the distinction is very subtle and already broken
in
> > many places. So I think it would be nicer if we can unify them into
> > ContextLifecycleObserver.
> 
> Doesn't this mean an initial empty document wouldn't be able to use web APIs
> that are supplied by supplements, such as VR, NFC, beacon, bluetooth, etc?

Yes, it means that.

I think I need to understand what the initial empty document needs to support.
Does it need to support full web APIs?

[dcheng, 2016-12-18 18:08:04]

On 2016/12/18 13:34:59, haraken wrote:
> On 2016/12/17 20:25:43, dcheng wrote:
> > On 2016/12/17 02:07:56, haraken wrote:
> > > On 2016/12/16 18:13:13, dcheng wrote:
> > > > Isn't the root cause that we're calling contextDestroyed() when it's not
> > > really
> > > > destroyed? It does seem like the initial empty document should still
have
> > > > supplements.
> > > 
> > > We're calling contextDestroyed because the initial empty document is
> actually
> > > destroyed :) Note that the frame is reused but the document is destroyed.
> > > 
> > > One solution is to keep both LocalFrameLifecycleObserver and
> > > ContextLifecycleObserver and ask people to use LocalFrameLifecycleObserver
> for
> > a
> > > per-frame thing. However, the distinction is very subtle and already
broken
> in
> > > many places. So I think it would be nicer if we can unify them into
> > > ContextLifecycleObserver.
> > 
> > Doesn't this mean an initial empty document wouldn't be able to use web APIs
> > that are supplied by supplements, such as VR, NFC, beacon, bluetooth, etc?
> 
> Yes, it means that.
> 
> I think I need to understand what the initial empty document needs to support.
> Does it need to support full web APIs?

Yeah, I think so. For example, if you load this document:

<body>
<iframe></iframe>
</body>

Then the <iframe> is loaded, but is on the initial empty document. Content can
subsequently be injected with things like innerHTML:
window[0].document.body.innerHTML = "<script>/* something that uses bluetooth
*/</script>";

Another way to end up with the initial empty document is:

<script>
var w = window.open();
w.document.body.innerHTML = "<script>/* something that uses VR */</script>";

None of the specs call out the initial empty document as unsupported in this
respect, so I think we have to support it.

[haraken, 2016-12-19 01:20:07]

On 2016/12/18 18:08:04, dcheng wrote:
> On 2016/12/18 13:34:59, haraken wrote:
> > On 2016/12/17 20:25:43, dcheng wrote:
> > > On 2016/12/17 02:07:56, haraken wrote:
> > > > On 2016/12/16 18:13:13, dcheng wrote:
> > > > > Isn't the root cause that we're calling contextDestroyed() when it's
not
> > > > really
> > > > > destroyed? It does seem like the initial empty document should still
> have
> > > > > supplements.
> > > > 
> > > > We're calling contextDestroyed because the initial empty document is
> > actually
> > > > destroyed :) Note that the frame is reused but the document is
destroyed.
> > > > 
> > > > One solution is to keep both LocalFrameLifecycleObserver and
> > > > ContextLifecycleObserver and ask people to use
LocalFrameLifecycleObserver
> > for
> > > a
> > > > per-frame thing. However, the distinction is very subtle and already
> broken
> > in
> > > > many places. So I think it would be nicer if we can unify them into
> > > > ContextLifecycleObserver.
> > > 
> > > Doesn't this mean an initial empty document wouldn't be able to use web
APIs
> > > that are supplied by supplements, such as VR, NFC, beacon, bluetooth, etc?
> > 
> > Yes, it means that.
> > 
> > I think I need to understand what the initial empty document needs to
support.
> > Does it need to support full web APIs?
> 
> Yeah, I think so. For example, if you load this document:
> 
> <body>
> <iframe></iframe>
> </body>
> 
> Then the <iframe> is loaded, but is on the initial empty document. Content can
> subsequently be injected with things like innerHTML:
> window[0].document.body.innerHTML = "<script>/* something that uses bluetooth
> */</script>";
> 
> Another way to end up with the initial empty document is:
> 
> <script>
> var w = window.open();
> w.document.body.innerHTML = "<script>/* something that uses VR */</script>";
> 
> None of the specs call out the initial empty document as unsupported in this
> respect, so I think we have to support it.

Thanks, just to help me understand more, in the above examples, when is
prepareForCommit called exactly?

[dcheng, 2016-12-19 01:27:09]

On 2016/12/19 01:20:07, haraken wrote:
> On 2016/12/18 18:08:04, dcheng wrote:
> > On 2016/12/18 13:34:59, haraken wrote:
> > > On 2016/12/17 20:25:43, dcheng wrote:
> > > > On 2016/12/17 02:07:56, haraken wrote:
> > > > > On 2016/12/16 18:13:13, dcheng wrote:
> > > > > > Isn't the root cause that we're calling contextDestroyed() when it's
> not
> > > > > really
> > > > > > destroyed? It does seem like the initial empty document should still
> > have
> > > > > > supplements.
> > > > > 
> > > > > We're calling contextDestroyed because the initial empty document is
> > > actually
> > > > > destroyed :) Note that the frame is reused but the document is
> destroyed.
> > > > > 
> > > > > One solution is to keep both LocalFrameLifecycleObserver and
> > > > > ContextLifecycleObserver and ask people to use
> LocalFrameLifecycleObserver
> > > for
> > > > a
> > > > > per-frame thing. However, the distinction is very subtle and already
> > broken
> > > in
> > > > > many places. So I think it would be nicer if we can unify them into
> > > > > ContextLifecycleObserver.
> > > > 
> > > > Doesn't this mean an initial empty document wouldn't be able to use web
> APIs
> > > > that are supplied by supplements, such as VR, NFC, beacon, bluetooth,
etc?
> > > 
> > > Yes, it means that.
> > > 
> > > I think I need to understand what the initial empty document needs to
> support.
> > > Does it need to support full web APIs?
> > 
> > Yeah, I think so. For example, if you load this document:
> > 
> > <body>
> > <iframe></iframe>
> > </body>
> > 
> > Then the <iframe> is loaded, but is on the initial empty document. Content
can
> > subsequently be injected with things like innerHTML:
> > window[0].document.body.innerHTML = "<script>/* something that uses
bluetooth
> > */</script>";
> > 
> > Another way to end up with the initial empty document is:
> > 
> > <script>
> > var w = window.open();
> > w.document.body.innerHTML = "<script>/* something that uses VR */</script>";
> > 
> > None of the specs call out the initial empty document as unsupported in this
> > respect, so I think we have to support it.
> 
> Thanks, just to help me understand more, in the above examples, when is
> prepareForCommit called exactly?

In the above examples, prepareForCommit() will be called only once, before we
load the initial empty document. However, before we load the initial empty
document, there is no document attached, so prepareForCommit() won't do very
much in that case.

However, any subsequent navigation will call prepareForCommit() -- and if the
document we will load is same-origin to the initial empty document, then we will
reuse the Window object.

[haraken, 2016-12-19 01:30:53]

On 2016/12/19 01:27:09, dcheng wrote:
> On 2016/12/19 01:20:07, haraken wrote:
> > On 2016/12/18 18:08:04, dcheng wrote:
> > > On 2016/12/18 13:34:59, haraken wrote:
> > > > On 2016/12/17 20:25:43, dcheng wrote:
> > > > > On 2016/12/17 02:07:56, haraken wrote:
> > > > > > On 2016/12/16 18:13:13, dcheng wrote:
> > > > > > > Isn't the root cause that we're calling contextDestroyed() when
it's
> > not
> > > > > > really
> > > > > > > destroyed? It does seem like the initial empty document should
still
> > > have
> > > > > > > supplements.
> > > > > > 
> > > > > > We're calling contextDestroyed because the initial empty document is
> > > > actually
> > > > > > destroyed :) Note that the frame is reused but the document is
> > destroyed.
> > > > > > 
> > > > > > One solution is to keep both LocalFrameLifecycleObserver and
> > > > > > ContextLifecycleObserver and ask people to use
> > LocalFrameLifecycleObserver
> > > > for
> > > > > a
> > > > > > per-frame thing. However, the distinction is very subtle and already
> > > broken
> > > > in
> > > > > > many places. So I think it would be nicer if we can unify them into
> > > > > > ContextLifecycleObserver.
> > > > > 
> > > > > Doesn't this mean an initial empty document wouldn't be able to use
web
> > APIs
> > > > > that are supplied by supplements, such as VR, NFC, beacon, bluetooth,
> etc?
> > > > 
> > > > Yes, it means that.
> > > > 
> > > > I think I need to understand what the initial empty document needs to
> > support.
> > > > Does it need to support full web APIs?
> > > 
> > > Yeah, I think so. For example, if you load this document:
> > > 
> > > <body>
> > > <iframe></iframe>
> > > </body>
> > > 
> > > Then the <iframe> is loaded, but is on the initial empty document. Content
> can
> > > subsequently be injected with things like innerHTML:
> > > window[0].document.body.innerHTML = "<script>/* something that uses
> bluetooth
> > > */</script>";
> > > 
> > > Another way to end up with the initial empty document is:
> > > 
> > > <script>
> > > var w = window.open();
> > > w.document.body.innerHTML = "<script>/* something that uses VR
*/</script>";
> > > 
> > > None of the specs call out the initial empty document as unsupported in
this
> > > respect, so I think we have to support it.
> > 
> > Thanks, just to help me understand more, in the above examples, when is
> > prepareForCommit called exactly?
> 
> In the above examples, prepareForCommit() will be called only once, before we
> load the initial empty document. However, before we load the initial empty
> document, there is no document attached, so prepareForCommit() won't do very
> much in that case.
> 
> However, any subsequent navigation will call prepareForCommit() -- and if the
> document we will load is same-origin to the initial empty document, then we
will
> reuse the Window object.

Thanks.

Do you think it's an option to not call contextDestroyed() when the initial
empty document navigates to a same-origin document (and thus reuse the window
object)? (Maybe is it what you suggested in #12?)

[dcheng, 2016-12-19 01:37:57]

On 2016/12/19 01:30:53, haraken wrote:
> On 2016/12/19 01:27:09, dcheng wrote:
> > On 2016/12/19 01:20:07, haraken wrote:
> > > On 2016/12/18 18:08:04, dcheng wrote:
> > > > On 2016/12/18 13:34:59, haraken wrote:
> > > > > On 2016/12/17 20:25:43, dcheng wrote:
> > > > > > On 2016/12/17 02:07:56, haraken wrote:
> > > > > > > On 2016/12/16 18:13:13, dcheng wrote:
> > > > > > > > Isn't the root cause that we're calling contextDestroyed() when
> it's
> > > not
> > > > > > > really
> > > > > > > > destroyed? It does seem like the initial empty document should
> still
> > > > have
> > > > > > > > supplements.
> > > > > > > 
> > > > > > > We're calling contextDestroyed because the initial empty document
is
> > > > > actually
> > > > > > > destroyed :) Note that the frame is reused but the document is
> > > destroyed.
> > > > > > > 
> > > > > > > One solution is to keep both LocalFrameLifecycleObserver and
> > > > > > > ContextLifecycleObserver and ask people to use
> > > LocalFrameLifecycleObserver
> > > > > for
> > > > > > a
> > > > > > > per-frame thing. However, the distinction is very subtle and
already
> > > > broken
> > > > > in
> > > > > > > many places. So I think it would be nicer if we can unify them
into
> > > > > > > ContextLifecycleObserver.
> > > > > > 
> > > > > > Doesn't this mean an initial empty document wouldn't be able to use
> web
> > > APIs
> > > > > > that are supplied by supplements, such as VR, NFC, beacon,
bluetooth,
> > etc?
> > > > > 
> > > > > Yes, it means that.
> > > > > 
> > > > > I think I need to understand what the initial empty document needs to
> > > support.
> > > > > Does it need to support full web APIs?
> > > > 
> > > > Yeah, I think so. For example, if you load this document:
> > > > 
> > > > <body>
> > > > <iframe></iframe>
> > > > </body>
> > > > 
> > > > Then the <iframe> is loaded, but is on the initial empty document.
Content
> > can
> > > > subsequently be injected with things like innerHTML:
> > > > window[0].document.body.innerHTML = "<script>/* something that uses
> > bluetooth
> > > > */</script>";
> > > > 
> > > > Another way to end up with the initial empty document is:
> > > > 
> > > > <script>
> > > > var w = window.open();
> > > > w.document.body.innerHTML = "<script>/* something that uses VR
> */</script>";
> > > > 
> > > > None of the specs call out the initial empty document as unsupported in
> this
> > > > respect, so I think we have to support it.
> > > 
> > > Thanks, just to help me understand more, in the above examples, when is
> > > prepareForCommit called exactly?
> > 
> > In the above examples, prepareForCommit() will be called only once, before
we
> > load the initial empty document. However, before we load the initial empty
> > document, there is no document attached, so prepareForCommit() won't do very
> > much in that case.
> > 
> > However, any subsequent navigation will call prepareForCommit() -- and if
the
> > document we will load is same-origin to the initial empty document, then we
> will
> > reuse the Window object.
> 
> Thanks.
> 
> Do you think it's an option to not call contextDestroyed() when the initial
> empty document navigates to a same-origin document (and thus reuse the window
> object)? (Maybe is it what you suggested in #12?)

No, I think that's probably not an option either: I think some of things are
context observers, because they're specced to cancel certain operations if the
document they are in unloads =(

[haraken, 2016-12-19 01:43:00]

On 2016/12/19 01:37:57, dcheng wrote:
> On 2016/12/19 01:30:53, haraken wrote:
> > On 2016/12/19 01:27:09, dcheng wrote:
> > > On 2016/12/19 01:20:07, haraken wrote:
> > > > On 2016/12/18 18:08:04, dcheng wrote:
> > > > > On 2016/12/18 13:34:59, haraken wrote:
> > > > > > On 2016/12/17 20:25:43, dcheng wrote:
> > > > > > > On 2016/12/17 02:07:56, haraken wrote:
> > > > > > > > On 2016/12/16 18:13:13, dcheng wrote:
> > > > > > > > > Isn't the root cause that we're calling contextDestroyed()
when
> > it's
> > > > not
> > > > > > > > really
> > > > > > > > > destroyed? It does seem like the initial empty document should
> > still
> > > > > have
> > > > > > > > > supplements.
> > > > > > > > 
> > > > > > > > We're calling contextDestroyed because the initial empty
document
> is
> > > > > > actually
> > > > > > > > destroyed :) Note that the frame is reused but the document is
> > > > destroyed.
> > > > > > > > 
> > > > > > > > One solution is to keep both LocalFrameLifecycleObserver and
> > > > > > > > ContextLifecycleObserver and ask people to use
> > > > LocalFrameLifecycleObserver
> > > > > > for
> > > > > > > a
> > > > > > > > per-frame thing. However, the distinction is very subtle and
> already
> > > > > broken
> > > > > > in
> > > > > > > > many places. So I think it would be nicer if we can unify them
> into
> > > > > > > > ContextLifecycleObserver.
> > > > > > > 
> > > > > > > Doesn't this mean an initial empty document wouldn't be able to
use
> > web
> > > > APIs
> > > > > > > that are supplied by supplements, such as VR, NFC, beacon,
> bluetooth,
> > > etc?
> > > > > > 
> > > > > > Yes, it means that.
> > > > > > 
> > > > > > I think I need to understand what the initial empty document needs
to
> > > > support.
> > > > > > Does it need to support full web APIs?
> > > > > 
> > > > > Yeah, I think so. For example, if you load this document:
> > > > > 
> > > > > <body>
> > > > > <iframe></iframe>
> > > > > </body>
> > > > > 
> > > > > Then the <iframe> is loaded, but is on the initial empty document.
> Content
> > > can
> > > > > subsequently be injected with things like innerHTML:
> > > > > window[0].document.body.innerHTML = "<script>/* something that uses
> > > bluetooth
> > > > > */</script>";
> > > > > 
> > > > > Another way to end up with the initial empty document is:
> > > > > 
> > > > > <script>
> > > > > var w = window.open();
> > > > > w.document.body.innerHTML = "<script>/* something that uses VR
> > */</script>";
> > > > > 
> > > > > None of the specs call out the initial empty document as unsupported
in
> > this
> > > > > respect, so I think we have to support it.
> > > > 
> > > > Thanks, just to help me understand more, in the above examples, when is
> > > > prepareForCommit called exactly?
> > > 
> > > In the above examples, prepareForCommit() will be called only once, before
> we
> > > load the initial empty document. However, before we load the initial empty
> > > document, there is no document attached, so prepareForCommit() won't do
very
> > > much in that case.
> > > 
> > > However, any subsequent navigation will call prepareForCommit() -- and if
> the
> > > document we will load is same-origin to the initial empty document, then
we
> > will
> > > reuse the Window object.
> > 
> > Thanks.
> > 
> > Do you think it's an option to not call contextDestroyed() when the initial
> > empty document navigates to a same-origin document (and thus reuse the
window
> > object)? (Maybe is it what you suggested in #12?)
> 
> No, I think that's probably not an option either: I think some of things are
> context observers, because they're specced to cancel certain operations if the
> document they are in unloads =(

Hmm... Then it's really hard to reason about what objects should be
ContextLifecycleObservers and what objects should be DOMWindowProperty :/

[haraken, 2016-12-19 01:44:49]

On 2016/12/19 01:43:00, haraken wrote:
> On 2016/12/19 01:37:57, dcheng wrote:
> > On 2016/12/19 01:30:53, haraken wrote:
> > > On 2016/12/19 01:27:09, dcheng wrote:
> > > > On 2016/12/19 01:20:07, haraken wrote:
> > > > > On 2016/12/18 18:08:04, dcheng wrote:
> > > > > > On 2016/12/18 13:34:59, haraken wrote:
> > > > > > > On 2016/12/17 20:25:43, dcheng wrote:
> > > > > > > > On 2016/12/17 02:07:56, haraken wrote:
> > > > > > > > > On 2016/12/16 18:13:13, dcheng wrote:
> > > > > > > > > > Isn't the root cause that we're calling contextDestroyed()
> when
> > > it's
> > > > > not
> > > > > > > > > really
> > > > > > > > > > destroyed? It does seem like the initial empty document
should
> > > still
> > > > > > have
> > > > > > > > > > supplements.
> > > > > > > > > 
> > > > > > > > > We're calling contextDestroyed because the initial empty
> document
> > is
> > > > > > > actually
> > > > > > > > > destroyed :) Note that the frame is reused but the document is
> > > > > destroyed.
> > > > > > > > > 
> > > > > > > > > One solution is to keep both LocalFrameLifecycleObserver and
> > > > > > > > > ContextLifecycleObserver and ask people to use
> > > > > LocalFrameLifecycleObserver
> > > > > > > for
> > > > > > > > a
> > > > > > > > > per-frame thing. However, the distinction is very subtle and
> > already
> > > > > > broken
> > > > > > > in
> > > > > > > > > many places. So I think it would be nicer if we can unify them
> > into
> > > > > > > > > ContextLifecycleObserver.
> > > > > > > > 
> > > > > > > > Doesn't this mean an initial empty document wouldn't be able to
> use
> > > web
> > > > > APIs
> > > > > > > > that are supplied by supplements, such as VR, NFC, beacon,
> > bluetooth,
> > > > etc?
> > > > > > > 
> > > > > > > Yes, it means that.
> > > > > > > 
> > > > > > > I think I need to understand what the initial empty document needs
> to
> > > > > support.
> > > > > > > Does it need to support full web APIs?
> > > > > > 
> > > > > > Yeah, I think so. For example, if you load this document:
> > > > > > 
> > > > > > <body>
> > > > > > <iframe></iframe>
> > > > > > </body>
> > > > > > 
> > > > > > Then the <iframe> is loaded, but is on the initial empty document.
> > Content
> > > > can
> > > > > > subsequently be injected with things like innerHTML:
> > > > > > window[0].document.body.innerHTML = "<script>/* something that uses
> > > > bluetooth
> > > > > > */</script>";
> > > > > > 
> > > > > > Another way to end up with the initial empty document is:
> > > > > > 
> > > > > > <script>
> > > > > > var w = window.open();
> > > > > > w.document.body.innerHTML = "<script>/* something that uses VR
> > > */</script>";
> > > > > > 
> > > > > > None of the specs call out the initial empty document as unsupported
> in
> > > this
> > > > > > respect, so I think we have to support it.
> > > > > 
> > > > > Thanks, just to help me understand more, in the above examples, when
is
> > > > > prepareForCommit called exactly?
> > > > 
> > > > In the above examples, prepareForCommit() will be called only once,
before
> > we
> > > > load the initial empty document. However, before we load the initial
empty
> > > > document, there is no document attached, so prepareForCommit() won't do
> very
> > > > much in that case.
> > > > 
> > > > However, any subsequent navigation will call prepareForCommit() -- and
if
> > the
> > > > document we will load is same-origin to the initial empty document, then
> we
> > > will
> > > > reuse the Window object.
> > > 
> > > Thanks.
> > > 
> > > Do you think it's an option to not call contextDestroyed() when the
initial
> > > empty document navigates to a same-origin document (and thus reuse the
> window
> > > object)? (Maybe is it what you suggested in #12?)
> > 
> > No, I think that's probably not an option either: I think some of things are
> > context observers, because they're specced to cancel certain operations if
the
> > document they are in unloads =(
> 
> Hmm... Then it's really hard to reason about what objects should be
> ContextLifecycleObservers and what objects should be DOMWindowProperty :/

If supplements are the only problem, maybe can we inject supplements every
navigation (including the navigation from an initial empty document to another
same-origin document)?

[haraken, 2016-12-19 02:31:35]

The CQ bit was checked by haraken@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 02:31:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 03:49:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-19 03:49:42]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-12-19 04:50:17]

On 2016/12/19 01:44:49, haraken wrote:
> On 2016/12/19 01:43:00, haraken wrote:
> > On 2016/12/19 01:37:57, dcheng wrote:
> > > On 2016/12/19 01:30:53, haraken wrote:
> > > > On 2016/12/19 01:27:09, dcheng wrote:
> > > > > On 2016/12/19 01:20:07, haraken wrote:
> > > > > > On 2016/12/18 18:08:04, dcheng wrote:
> > > > > > > On 2016/12/18 13:34:59, haraken wrote:
> > > > > > > > On 2016/12/17 20:25:43, dcheng wrote:
> > > > > > > > > On 2016/12/17 02:07:56, haraken wrote:
> > > > > > > > > > On 2016/12/16 18:13:13, dcheng wrote:
> > > > > > > > > > > Isn't the root cause that we're calling contextDestroyed()
> > when
> > > > it's
> > > > > > not
> > > > > > > > > > really
> > > > > > > > > > > destroyed? It does seem like the initial empty document
> should
> > > > still
> > > > > > > have
> > > > > > > > > > > supplements.
> > > > > > > > > > 
> > > > > > > > > > We're calling contextDestroyed because the initial empty
> > document
> > > is
> > > > > > > > actually
> > > > > > > > > > destroyed :) Note that the frame is reused but the document
is
> > > > > > destroyed.
> > > > > > > > > > 
> > > > > > > > > > One solution is to keep both LocalFrameLifecycleObserver and
> > > > > > > > > > ContextLifecycleObserver and ask people to use
> > > > > > LocalFrameLifecycleObserver
> > > > > > > > for
> > > > > > > > > a
> > > > > > > > > > per-frame thing. However, the distinction is very subtle and
> > > already
> > > > > > > broken
> > > > > > > > in
> > > > > > > > > > many places. So I think it would be nicer if we can unify
them
> > > into
> > > > > > > > > > ContextLifecycleObserver.
> > > > > > > > > 
> > > > > > > > > Doesn't this mean an initial empty document wouldn't be able
to
> > use
> > > > web
> > > > > > APIs
> > > > > > > > > that are supplied by supplements, such as VR, NFC, beacon,
> > > bluetooth,
> > > > > etc?
> > > > > > > > 
> > > > > > > > Yes, it means that.
> > > > > > > > 
> > > > > > > > I think I need to understand what the initial empty document
needs
> > to
> > > > > > support.
> > > > > > > > Does it need to support full web APIs?
> > > > > > > 
> > > > > > > Yeah, I think so. For example, if you load this document:
> > > > > > > 
> > > > > > > <body>
> > > > > > > <iframe></iframe>
> > > > > > > </body>
> > > > > > > 
> > > > > > > Then the <iframe> is loaded, but is on the initial empty document.
> > > Content
> > > > > can
> > > > > > > subsequently be injected with things like innerHTML:
> > > > > > > window[0].document.body.innerHTML = "<script>/* something that
uses
> > > > > bluetooth
> > > > > > > */</script>";
> > > > > > > 
> > > > > > > Another way to end up with the initial empty document is:
> > > > > > > 
> > > > > > > <script>
> > > > > > > var w = window.open();
> > > > > > > w.document.body.innerHTML = "<script>/* something that uses VR
> > > > */</script>";
> > > > > > > 
> > > > > > > None of the specs call out the initial empty document as
unsupported
> > in
> > > > this
> > > > > > > respect, so I think we have to support it.
> > > > > > 
> > > > > > Thanks, just to help me understand more, in the above examples, when
> is
> > > > > > prepareForCommit called exactly?
> > > > > 
> > > > > In the above examples, prepareForCommit() will be called only once,
> before
> > > we
> > > > > load the initial empty document. However, before we load the initial
> empty
> > > > > document, there is no document attached, so prepareForCommit() won't
do
> > very
> > > > > much in that case.
> > > > > 
> > > > > However, any subsequent navigation will call prepareForCommit() -- and
> if
> > > the
> > > > > document we will load is same-origin to the initial empty document,
then
> > we
> > > > will
> > > > > reuse the Window object.
> > > > 
> > > > Thanks.
> > > > 
> > > > Do you think it's an option to not call contextDestroyed() when the
> initial
> > > > empty document navigates to a same-origin document (and thus reuse the
> > window
> > > > object)? (Maybe is it what you suggested in #12?)
> > > 
> > > No, I think that's probably not an option either: I think some of things
are
> > > context observers, because they're specced to cancel certain operations if
> the
> > > document they are in unloads =(
> > 
> > Hmm... Then it's really hard to reason about what objects should be
> > ContextLifecycleObservers and what objects should be DOMWindowProperty :/
> 
> If supplements are the only problem, maybe can we inject supplements every
> navigation (including the navigation from an initial empty document to another
> same-origin document)?

PS4 is trying the above approach. It passes all tests.

[dcheng, 2016-12-19 04:53:51]

dcheng@chromium.org changed reviewers:
+ domenic@chromium.org

[dcheng, 2016-12-19 04:53:51]

On 2016/12/19 01:44:49, haraken wrote:
> On 2016/12/19 01:43:00, haraken wrote:
> > On 2016/12/19 01:37:57, dcheng wrote:
> > > On 2016/12/19 01:30:53, haraken wrote:
> > > > On 2016/12/19 01:27:09, dcheng wrote:
> > > > > On 2016/12/19 01:20:07, haraken wrote:
> > > > > > On 2016/12/18 18:08:04, dcheng wrote:
> > > > > > > On 2016/12/18 13:34:59, haraken wrote:
> > > > > > > > On 2016/12/17 20:25:43, dcheng wrote:
> > > > > > > > > On 2016/12/17 02:07:56, haraken wrote:
> > > > > > > > > > On 2016/12/16 18:13:13, dcheng wrote:
> > > > > > > > > > > Isn't the root cause that we're calling contextDestroyed()
> > when
> > > > it's
> > > > > > not
> > > > > > > > > > really
> > > > > > > > > > > destroyed? It does seem like the initial empty document
> should
> > > > still
> > > > > > > have
> > > > > > > > > > > supplements.
> > > > > > > > > > 
> > > > > > > > > > We're calling contextDestroyed because the initial empty
> > document
> > > is
> > > > > > > > actually
> > > > > > > > > > destroyed :) Note that the frame is reused but the document
is
> > > > > > destroyed.
> > > > > > > > > > 
> > > > > > > > > > One solution is to keep both LocalFrameLifecycleObserver and
> > > > > > > > > > ContextLifecycleObserver and ask people to use
> > > > > > LocalFrameLifecycleObserver
> > > > > > > > for
> > > > > > > > > a
> > > > > > > > > > per-frame thing. However, the distinction is very subtle and
> > > already
> > > > > > > broken
> > > > > > > > in
> > > > > > > > > > many places. So I think it would be nicer if we can unify
them
> > > into
> > > > > > > > > > ContextLifecycleObserver.
> > > > > > > > > 
> > > > > > > > > Doesn't this mean an initial empty document wouldn't be able
to
> > use
> > > > web
> > > > > > APIs
> > > > > > > > > that are supplied by supplements, such as VR, NFC, beacon,
> > > bluetooth,
> > > > > etc?
> > > > > > > > 
> > > > > > > > Yes, it means that.
> > > > > > > > 
> > > > > > > > I think I need to understand what the initial empty document
needs
> > to
> > > > > > support.
> > > > > > > > Does it need to support full web APIs?
> > > > > > > 
> > > > > > > Yeah, I think so. For example, if you load this document:
> > > > > > > 
> > > > > > > <body>
> > > > > > > <iframe></iframe>
> > > > > > > </body>
> > > > > > > 
> > > > > > > Then the <iframe> is loaded, but is on the initial empty document.
> > > Content
> > > > > can
> > > > > > > subsequently be injected with things like innerHTML:
> > > > > > > window[0].document.body.innerHTML = "<script>/* something that
uses
> > > > > bluetooth
> > > > > > > */</script>";
> > > > > > > 
> > > > > > > Another way to end up with the initial empty document is:
> > > > > > > 
> > > > > > > <script>
> > > > > > > var w = window.open();
> > > > > > > w.document.body.innerHTML = "<script>/* something that uses VR
> > > > */</script>";
> > > > > > > 
> > > > > > > None of the specs call out the initial empty document as
unsupported
> > in
> > > > this
> > > > > > > respect, so I think we have to support it.
> > > > > > 
> > > > > > Thanks, just to help me understand more, in the above examples, when
> is
> > > > > > prepareForCommit called exactly?
> > > > > 
> > > > > In the above examples, prepareForCommit() will be called only once,
> before
> > > we
> > > > > load the initial empty document. However, before we load the initial
> empty
> > > > > document, there is no document attached, so prepareForCommit() won't
do
> > very
> > > > > much in that case.
> > > > > 
> > > > > However, any subsequent navigation will call prepareForCommit() -- and
> if
> > > the
> > > > > document we will load is same-origin to the initial empty document,
then
> > we
> > > > will
> > > > > reuse the Window object.
> > > > 
> > > > Thanks.
> > > > 
> > > > Do you think it's an option to not call contextDestroyed() when the
> initial
> > > > empty document navigates to a same-origin document (and thus reuse the
> > window
> > > > object)? (Maybe is it what you suggested in #12?)
> > > 
> > > No, I think that's probably not an option either: I think some of things
are
> > > context observers, because they're specced to cancel certain operations if
> the
> > > document they are in unloads =(
> > 
> > Hmm... Then it's really hard to reason about what objects should be
> > ContextLifecycleObservers and what objects should be DOMWindowProperty :/
> 
> If supplements are the only problem, maybe can we inject supplements every
> navigation (including the navigation from an initial empty document to another
> same-origin document)?

I think it's worth trying. I personally can't imagine a reason why supplements
should be reused in this case, but it's possible there's some strange edge cases
out there...

+domenic to see if he has any spec [1] insight here.

[1] The main reference I've used for this is
https://html.spec.whatwg.org/multipage/browsers.html#windows, I'm not sure if
there are others.

[dcheng, 2016-12-19 04:56:28]

ps4 lgtm, but i guess the CL description will need to be updated =)

[haraken, 2016-12-19 05:09:20]

Description was changed from

==========
Don't install supplements on the initial empty document

Currently we install supplement objects on the initial empty document.          
                       
This is problematic for supplement objects that inherit from                    
                    
ContextLifecycleObserver:                                                       
                    

1) The frame displays the initial empty document.
2) The frame navigates to a new same-origin document. At this point,            
                    
   supplement objects that inherit from ContextLifecycleObserver receive        
                    
   contextDestroyed and stop working. The frame is reused and thus              
                    
   the supplement objects are also reused.                                      
                    
3) The new document tries to access the supplement objects and fails.
                                                                                
                    
This CL prevents the scenario by not installing supplement                      
         
objects to the initial empty document.

BUG=610176
==========

to

==========
Install supplements at every navigation

Currently supplement objects are reused when an initial empty document navigates
to
a same-origin document. This is problematic because ContextLifecycleObservers
are
detached when the initial empty document is destroyed. They cannot be used after
the navigation.

To fix the problem, we need to install supplements at every navigation.

BUG=610176
==========

[haraken, 2016-12-19 05:09:34]

On 2016/12/19 04:56:28, dcheng wrote:
> ps4 lgtm, but i guess the CL description will need to be updated =)

Thanks, updated the CL description.

[haraken, 2016-12-19 05:09:39]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-12-19 05:09:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 05:14:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482124179027040,
"parent_rev": "4bd5eb8f4ea02ffa701c1f3a581cec52c497d2ea", "commit_rev":
"f1f4adaf40c8b98dd52f8628b2e7b01b52cbc80b"}

[commit-bot: I haz the power, 2016-12-19 05:15:28]

Description was changed from

==========
Install supplements at every navigation

Currently supplement objects are reused when an initial empty document navigates
to
a same-origin document. This is problematic because ContextLifecycleObservers
are
detached when the initial empty document is destroyed. They cannot be used after
the navigation.

To fix the problem, we need to install supplements at every navigation.

BUG=610176
==========

to

==========
Install supplements at every navigation

Currently supplement objects are reused when an initial empty document navigates
to
a same-origin document. This is problematic because ContextLifecycleObservers
are
detached when the initial empty document is destroyed. They cannot be used after
the navigation.

To fix the problem, we need to install supplements at every navigation.

BUG=610176

Review-Url: https://codereview.chromium.org/2580753004
==========

[commit-bot: I haz the power, 2016-12-19 05:15:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-19 05:20:05]

Description was changed from

==========
Install supplements at every navigation

Currently supplement objects are reused when an initial empty document navigates
to
a same-origin document. This is problematic because ContextLifecycleObservers
are
detached when the initial empty document is destroyed. They cannot be used after
the navigation.

To fix the problem, we need to install supplements at every navigation.

BUG=610176

Review-Url: https://codereview.chromium.org/2580753004
==========

to

==========
Install supplements at every navigation

Currently supplement objects are reused when an initial empty document navigates
to
a same-origin document. This is problematic because ContextLifecycleObservers
are
detached when the initial empty document is destroyed. They cannot be used after
the navigation.

To fix the problem, we need to install supplements at every navigation.

BUG=610176

Committed: https://crrev.com/dd2e0942b294948ee891b1e54ca17a195fc1f3a1
Cr-Commit-Position: refs/heads/master@{#439413}
==========

[commit-bot: I haz the power, 2016-12-19 05:20:07]

Patchset 4 (id:??) landed as
https://crrev.com/dd2e0942b294948ee891b1e54ca17a195fc1f3a1
Cr-Commit-Position: refs/heads/master@{#439413}