969156 - Don't fire onNewGlobalObject until the end of nsGlobalWindow::SetNewDocument

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bobby Holley (:bholley)]

See bug 964545 comment 19. I think I can whip up a quick patch for this.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Don't fire onNewGlobalObject until the end of nsGlobalWindow::SetNewDocument. v1

Erik, does this solve your problem?

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

https://tbpl.mozilla.org/?tree=Try&rev=1dadcfddafc0

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #2)
> https://tbpl.mozilla.org/?tree=Try&rev=1dadcfddafc0

Oops ignore the above

I'm testing your patch and https://github.com/erikvold/gecko-dev/compare/964545v7 at https://tbpl.mozilla.org/?tree=Try&rev=7b2c43093824

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8371961 [details] [diff] [review]
Don't fire onNewGlobalObject until the end of nsGlobalWindow::SetNewDocument. v1

It seems to work \o/ https://tbpl.mozilla.org/?tree=Try&rev=7b2c43093824

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8371961 [details] [diff] [review]
Don't fire onNewGlobalObject until the end of nsGlobalWindow::SetNewDocument. v1

Great! Flagging Boris for review.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8371961 [details] [diff] [review]
Don't fire onNewGlobalObject until the end of nsGlobalWindow::SetNewDocument. v1

So what worries me is sending this notification after DispatchDOMWindowCreated.  Wouldn't it be better to do it before people have had a chance to poke at the window?

Do we need to enter the compartment of global before calling JS_FireOnNewGlobalObject?

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> So what worries me is sending this notification after
> DispatchDOMWindowCreated.  Wouldn't it be better to do it before people have
> had a chance to poke at the window?

Yeah, that's a good point. I'll fix that.

> Do we need to enter the compartment of global before calling
> JS_FireOnNewGlobalObject?

We're already in the compartment of the outer (which is the compartment of the inner, at this point) a bit further up in the function.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Don't fire onNewGlobalObject until the end of nsGlobalWindow::SetNewDocument. v2

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8372767 [details] [diff] [review]
Don't fire onNewGlobalObject until the end of nsGlobalWindow::SetNewDocument. v2

r=me

Comment 10

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a77c3bb10394

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a77c3bb10394

Comment 12

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #4)
> Comment on attachment 8371961 [details] [diff] [review]
> Don't fire onNewGlobalObject until the end of
> nsGlobalWindow::SetNewDocument. v1
> 
> It seems to work \o/ https://tbpl.mozilla.org/?tree=Try&rev=7b2c43093824

So v1 seemed to work, and v2 seems to not work.

See the try mentioned here: https://bugzilla.mozilla.org/show_bug.cgi?id=964545#c21

Comment 13

[Bobby Holley (:bholley)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #12) 
> So v1 seemed to work, and v2 seems to not work.

Are you sure? I'd be quite surprised if the failure mode were the same. The problem before was that we were invoking onNewGlobalObject before the new inner window was hooked up to the outer. With this patch, that work has definitely happened. The difference between v1 and v2 is purely the difference between firing onNewGlobalObject before or after DomWindowCreated.
 
> See the try mentioned here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=964545#c21

That try run appears green to me.

Comment 14

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Bobby Holley (:bholley) from comment #13)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #12) 
> > So v1 seemed to work, and v2 seems to not work.
> 
> Are you sure? I'd be quite surprised if the failure mode were the same. The
> problem before was that we were invoking onNewGlobalObject before the new
> inner window was hooked up to the outer. With this patch, that work has
> definitely happened. The difference between v1 and v2 is purely the
> difference between firing onNewGlobalObject before or after DomWindowCreated.
>  
> > See the try mentioned here:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=964545#c21
> 
> That try run appears green to me.

Hmm I tried again https://bugzilla.mozilla.org/show_bug.cgi?id=964545#c22 and it did work!