Web compat issues with window.open

RESOLVED FIXED in Firefox OS v2.1

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kgrandon, Assigned: gduan)

Tracking

unspecified
2.1 S5 (26sep)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(4 attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Created attachment 8481024 [details] [review]
Pull request - switch from activities to appWindow.requestOpen
(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?
(Reporter)

Comment 3

4 years ago
Created attachment 8481077 [details] [review]
Pull request - add opener test case

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?

Updated

4 years ago
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)
Created attachment 8490228 [details] [review]
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Please add a Gaia v2.1 nomination when you get a chance :)
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
Flags: needinfo?(gduan)
Created attachment 8496650 [details] [review]
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: 1078707
Depends on: 1084321

Comment 17

4 years ago
Hello Kevin,
Would you please offer me a full step, and please tell which expression is fail.
Thanks!
Flags: needinfo?(kgrandon)
(Reporter)

Comment 18

4 years ago
(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.