Closed Bug 1060191 Opened 10 years ago Closed 10 years ago

Web compat issues with window.open

Categories

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.1+, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S5 (26sep)
blocking-b2g 2.1+
Tracking Status
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: kgrandon, Assigned: gduan)

References

Details

Attachments

(4 files)

Currently the way gaia opens new windows has many issues for web compatibility. Here are a few known issues:

- It's likely that we will open the same URL or named window twice.
- We do not open windows in the same process.
- We probably do not have a proper reference to window.opener.
(In reply to Kevin Grandon :kgrandon from comment #0)
> Currently the way gaia opens new windows has many issues for web
> compatibility. Here are a few known issues:
> 
> - It's likely that we will open the same URL or named window twice.
> - We do not open windows in the same process.
> - We probably do not have a proper reference to window.opener.

I don't understand the problem here. The window names are tracked in Gecko and Gecko will silently re-use the popup window in the same process to load the new content. So this should work and system app should only receive mozbrowseropenwindow once:

var win = window.open('https://www.mozilla.org/#foo', 'moz');
var win2 = window.open('https://www.mozilla.org/#foo2', 'moz');

console.log(win === win2); // true
console.log(win2.location.href); // will be "https://www.mozilla.org/#foo2" assuming the frame is loaded

I think the problem only arise if we, instead of appending the passed frame element for the event to the DOM, we ditch it and do something else. In that case |win| will be null (since no "window" was created from the app's perspective).

Is that what we are doing right now? What do we do in Gaia to break the web?
I think the issue may only be related to windows with a name of _blank. I believe in this case the window.opener object should exist?

Here is a pull request which adds a failing test case. The window.opener object is null in the opened window.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #2)
> (In reply to Kevin Grandon :kgrandon from comment #0)
> > Currently the way gaia opens new windows has many issues for web
> > compatibility. Here are a few known issues:
> > 
> > - It's likely that we will open the same URL or named window twice.
> > - We do not open windows in the same process.
> > - We probably do not have a proper reference to window.opener.
> 
> I don't understand the problem here. The window names are tracked in Gecko
> and Gecko will silently re-use the popup window in the same process to load
> the new content. So this should work and system app should only receive
> mozbrowseropenwindow once:
> 
> var win = window.open('https://www.mozilla.org/#foo', 'moz');
> var win2 = window.open('https://www.mozilla.org/#foo2', 'moz');
> 
> console.log(win === win2); // true
> console.log(win2.location.href); // will be "https://www.mozilla.org/#foo2"
> assuming the frame is loaded
> 
> I think the problem only arise if we, instead of appending the passed frame
> element for the event to the DOM, we ditch it and do something else. In that
> case |win| will be null (since no "window" was created from the app's
> perspective).
> 
> Is that what we are doing right now? What do we do in Gaia to break the web?

What Kevin says. This happens specifically because of this code: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/child_window_factory.js#L57

The issue is that instead of reusing the window passed in the detail object of mozbrowseropenwindow, the Gaia codebase speficially open a new AppWindow, and load the asked url into it.

I think this issue is here for system bookmarks / e.me collection for a while, but we didn't pay attention to it, as it was working correctly in the browser app. Now that the browser app is moved back into the system app, this is much more painful as it may results into unexpected results for web site.
[Blocking Requested - why for this release]: Web Compatibility with window.open('foo.html', '_blank') is broken, and may results into unexpected web breakage...
blocking-b2g: --- → 2.1?
Assignee: nobody → alive
blocking-b2g: 2.1? → 2.1+
Target Milestone: --- → 2.1 S5 (26sep)
Could you steal this from me? A good bug.
Flags: needinfo?(gduan)
Assignee: alive → gduan
Flags: needinfo?(gduan)
Attached file PR to master
Patch with test from comment 3.
waiting for test result.
Comment on attachment 8490228 [details] [review]
PR to master

Hi Alive,
could you review this patch?
Thanks.
Attachment #8490228 - Flags: review?(alive)
Comment on attachment 8490228 [details] [review]
PR to master

f+, I'd like to let AWF to create all AppWindow instances.
Attachment #8490228 - Flags: review?(alive) → feedback+
Comment on attachment 8490228 [details] [review]
PR to master

Hi Alive,
could you take a look on this patch? Thanks.
Attachment #8490228 - Flags: review?(alive)
Comment on attachment 8490228 [details] [review]
PR to master

r+ with nits
Attachment #8490228 - Flags: review?(alive) → review+
PR to master
https://github.com/mozilla-b2g/gaia/commit/6163c5ee0f99f73439318d064e0862ccd4e11a49
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Please add a Gaia v2.1 nomination when you get a chance :)
Flags: needinfo?(gduan)
Attached file PR to 2.1
wait for test result.
Flags: needinfo?(gduan)
Comment on attachment 8496650 [details] [review]
PR to 2.1

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): No.
[User impact] if declined: As Comment 5, window.open('foo.html', '_blank') may result into  unexpected web breakage if some page use it.
[Testing completed]: Yes.
[Risk to taking this patch] (and alternatives if risky): No.
[String changes made]:
Attachment #8496650 - Flags: approval-gaia-v2.1?
Attachment #8496650 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
Depends on: 1084321
Hello Kevin,
Would you please offer me a full step, and please tell which expression is fail.
Thanks!
Flags: needinfo?(kgrandon)
(In reply to SandKing from comment #17)
> Hello Kevin,
> Would you please offer me a full step, and please tell which expression is
> fail.
> Thanks!

I'm not sure I understand the question here. If you want to verify the bug, you'll likely need to create a test page, or find one which references the window.opener property after a window.open().
Flags: needinfo?(kgrandon)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: