765075 - Calling close() immediately after window.open() on a window opened OOP by <iframe mozbrowser> happens before BrowserElementChild is loaded

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

If you do

  var w = window.open(...);
  w.close();

OOP, the close() happens before the new window's BrowserElementChild is created, because the BrowserElementChild is created only once the frame loader shows the window.  The "fake show" I added to TabChild as part of the window.open dance isn't good enough.

It's not as simple as creating the BrowserElementChild at the time we do the fake show, either; the message manager doesn't exist until we do the frame loader show.  Fun.

Comment 1

[Justin Lebar (not reading bugmail)]

My attempts to move the loadFrameScript("BrowserElementChild.js") earlier have not been successful.  Or at least, I can indeed load the frame script earlier, but it's not early enough.

The problem is that the parent sends the child a message saying "load this frame script".  I think I'll need to move to a system where the child loads the script itself.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

This is kind of ugly -- it would be cleaner to load BrowserElementChild.js exactly once -- but it's a much simpler change than I think I'd have to make otherwise.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 633681 [details] [diff] [review]
Patch v1

This is just too ugly.
I'm not sure how to keep a list of scripts to load asap, and
how to prevent parent to send them afterwards, but that is what should happen.

Comment 4

[Justin Lebar (not reading bugmail)]

I can't see how to do this in a simple manner.

The best thing I can think of is: For in-process iframes, do what we currently do.  For OOP iframes created from the frameloader, set a flag on the tabchild before we call show, saying "you're a browser frame".  For OOP iframes opened via window.open, manually set that flag during the window.open.  Then when we do TabChild::RecvShow, check the flag, and if it's set, instantiate the BrowserElementChild.

That's much more complicated than this current patch, and the benefits are small, if they exist at all.

I don't think this is worth wasting more time on.  The approach here isn't fragile -- it's not going to cause regressions down the line.  Code complete is in a month; I simply can't spend time on things that already work.

Comment 5

[Justin Lebar (not reading bugmail)]

> Code complete is in a month

I should say, feature complete is in a month, code complete in two months.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 633681 [details] [diff] [review]
Patch v1

Still makes no sense.

A temporary hack I could accept is to add some boolean parameter to
PBrowser constructor. If it is true, TabChild
would automatically load "chrome://global/content/BrowserElementChild.js"
asap after creation. Then remove the explicit
http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.js#163 and 
parent->CreateTab(chromeFlags, OwnerIsBrowserFrame()) in nsFrameLoader.

Comment 7

[Justin Lebar (not reading bugmail)]

I feel that hack is much uglier than what I have here, so I'll try to come up with something that we both agree is more elegant.

Comment 8

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

What do you think of this?

Comment 9

[Justin Lebar (not reading bugmail)]

smaug wants me to send the in-process load-script from C++ too, since he doesn't like sending in JS sometimes and C++ other times.

He also wants me to pass a boolean parameter for is-browser-frame in to the PBrowser constructor, instead of sending a message, because sending a message is "slow".

Comment 10

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3

Comment 11

[Justin Lebar (not reading bugmail)]

This landed and bounced because of a wide range of intermittent orange; see bug 764718 comment 26.

My guess is that the orange came from this bug, rather than bug 764718, but I need to investigate.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/18d056438d00

Comment 13

[Justin Lebar (not reading bugmail)]

But then this was backed out: https://hg.mozilla.org/mozilla-central/rev/1da4a83db7b2

Comment 14

[Justin Lebar (not reading bugmail)]

I can't reproduce this locally on Mac or Linux, so I've pushed to try with some extra debugging information enabled:

https://tbpl.mozilla.org/?tree=Try&rev=e6d95c9eb1f5
https://tbpl.mozilla.org/?tree=Try&rev=2120ae253449

Comment 15

[Justin Lebar (not reading bugmail)]

Gah, I pushed those with bad trychooser syntax, so no builds were run.  Trying again:

https://tbpl.mozilla.org/?tree=Try&rev=98321ce6f771
https://tbpl.mozilla.org/?tree=Try&rev=8afcbcae9f7d

Comment 16

[Justin Lebar (not reading bugmail)]

Interesting!

> JavaScript error: resource:///components/BrowserElementParent.js, line 176: can't access 
> dead object

From https://tbpl.mozilla.org/php/getParsedLog.php?id=12913528&tree=Try#error0 (full queue, this bug plus bug 764718).

Comment 17

[Justin Lebar (not reading bugmail)]

More try pushes:

https://tbpl.mozilla.org/?tree=Try&rev=a3cd3e86d48b <-- maybe fixes some tests

https://tbpl.mozilla.org/?tree=Try&rev=967dc9b52b4b <-- with patch v1 from this bug, to see if that has the same oranges

Comment 18

[Justin Lebar (not reading bugmail)]

I now think the problem is in bug 764718.

Comment 19

[Justin Lebar (not reading bugmail)]

I was wrong in comment 13: This was never backed out.