776129 - When mozbrowser does window.open, the opened window should be OOP iff the opener is OOP

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Dave Hylands [:dhylands]]

UI Test App window.open comes up with a rounded rectangle white window, much like bug 776086

Comment 1

[Dave Hylands [:dhylands]]

All core apps need to run OOP

Comment 2

[Justin Lebar (not reading bugmail)]

Ouch, I'm seeing

> [Parent 8899] ###!!! ASSERTION: ShowRemote only makes sense on remote frames.: 'mRemoteFrame', 
> file ../../../../src/content/base/src/nsFrameLoader.cpp, line 890

and later

> ###################################### forms.js loaded
> JavaScript error: chrome://global/content/BrowserElementChild.js, line 672: redeclaration of const 
> ContentPanning

Neither of these looks good!

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: WIP, v1

The problem was that we weren't forcing the frame created by window.open to run out-of-process!

This fixes things, but I need to figure out how to write a test, and also whether we have the inverse problem: Can in-process apps attempt to spawn out-of-process popups?  That would also be bad.

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Setting remote=true for OOP popups is neither necessary nor sufficient to fix the OOP bug; I set the attribute only because we have to set it in the in-process case, and it would be weird for content to see remote=false but never see remote=true.

Setting remote=true doesn't do anything because our OOP loading code (nsFrameLoaer::SetRemoteBrowser) doesn't ever call nsFrameLoader::MaybeCreateDocShell(), which is what would end up reading the remote attribute.  We could change the frameloader code to call MaybeCreateDocShell(), if you preferred, but I think this is clearer.

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Tests, v1

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Funny, I'm sitting on a patch to do this for bug 781725 :).

Comment 7

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

Comment on attachment 652474 [details] [diff] [review]
Patch, v1

I think CreateIframe should take 3rd parameter to say whether the element
is remote or not and set the attribute inside CreateIframe.

With that, r=me

Comment 8

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

Comment on attachment 652475 [details] [diff] [review]
Tests, v1


>+// We need to open OOP frames when the OOP-by-default pref is /off/, and we
>+// similarly need to open in-process frames when the OOP-by-default pref is
>+// /on/.
>+//
>+// Since the name of the test determines the OOP-by-default pref, the "inproc"
>+// version of this test opens an OOP frame, and the "oop" version opens an
>+// in-process frame.  Enjoy.  :)
This is über-confusing. Could you explain *why* OOP /off/ means need-to-open-OOP.
It seems that that is just what you're testing, but please say so.


/me crosses fingers this actually passes on try ;)

Comment 9

[Justin Lebar (not reading bugmail)]

> Could you explain *why* OOP /off/ means need-to-open-OOP.

You're right, I should be more explicit in the bug and say that it appears when you create an OOP frame when oop-by-default == false, or when you create an inproc frame when oop-by-default == true.

Thanks for the reviews!

Comment 10

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=8f7c13822529

Comment 11

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ade2d26d26e
https://hg.mozilla.org/integration/mozilla-inbound/rev/66e1e2f6f108

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ade2d26d26e
https://hg.mozilla.org/mozilla-central/rev/66e1e2f6f108

Comment 13

[Gene Lian (I already quit Mozilla)]

Hi Justin, could you please take a look at bug 778300? window.open() still doesn't work properly for us when calling it from an OOP app (Alarm-Clock). Is that because of the wrong calling of window.open()?

Comment 14

[Andreas Gal :gal]

Alarm should be a notification not window.open.

Comment 15

[Justin Lebar (not reading bugmail)]

(In reply to Andreas Gal :gal from comment #14)
> Alarm should be a notification not window.open.

Really?  Last time I checked, we showed the incoming call screen using window.open.  The alarm-is-sounding screen sounds pretty similar, at least at first glance.

Comment 16

[Andreas Gal :gal]

I guess up to the UX design. In any case, the underlying bug has to be fixed since it will hit the dialer case.