Closed Bug 776129 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(blocking-basecamp:+)

RESOLVED FIXED
blocking-basecamp +

People

(Reporter: dhylands, Assigned: justin.lebar+bug)

References

(Blocks 1 open bug)

Details

(Whiteboard: [LOE:S])

Attachments

(2 files, 1 obsolete file)

UI Test App window.open comes up with a rounded rectangle white window, much like bug 776086
All core apps need to run OOP
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Depends on: 782971
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!
Attached patch WIP, v1 (obsolete) — Splinter Review
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.
Assignee: nobody → justin.lebar+bug
Whiteboard: [LOE:S]
Blocks: 782400
Attached patch Patch, v1Splinter Review
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.
Attachment #652474 - Flags: review?(bugs)
Attached patch Tests, v1Splinter Review
Attachment #652475 - Flags: review?(bugs)
Attachment #652439 - Attachment is obsolete: true
Summary: UI Test - window.open doesn't work properly when run OOP → When mozbrowser does window.open, the opened window should be OOP iff the opener is OOP
Funny, I'm sitting on a patch to do this for bug 781725 :).
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
Attachment #652474 - Flags: review?(bugs) → review+
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 ;)
Attachment #652475 - Flags: review?(bugs) → review+
> 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!
Blocks: 783644
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()?
Alarm should be a notification not window.open.
(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.
I guess up to the UX design. In any case, the underlying bug has to be fixed since it will hit the dialer case.
Depends on: 820749
You need to log in before you can comment on or make changes to this bug.