725502 - window.open with dialog=yes argument in it causes weird issue

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, steps to reproduce:
- Have at least 2 tabs open
- Open the testcase, tap on the "windowopen dialog=yes buttononclick" button

Expected result:
- A new tab opens with that same page again

Actual result:
- No new tab is opened and you get weird issues, as if Fennec's tab management is confused.

See also bug 724780 and bug 725496, there might be some relation to those bugs.

Comment 1

[Matt Brubeck (:mbrubeck)]

Looking at the source code, "dialog=yes" and window.find seem to call nsIDOMWindow.openDialog, which doesn't do the right thing for Fennec.  Is there a way to override openDialog like we do for the nsIBrowserDOMWindow methods?  What does XUL Fennec do?

Comment 2

[Tony Chung [:tchung]]

mark, brad, 
can we get this bug triaged to block tracking-fennec+? ?   

This bug blocks any Fennec Native users trying to sign in on sites that implement current BrowserID flow. 

See bug 730015 and https://github.com/mozilla/browserid/issues/1169 for more info.

Tony

Comment 3

[Tony Chung [:tchung]]

(In reply to Tony Chung [:tchung] from comment #2)
> mark, brad, 
> can we get this bug triaged to block tracking-fennec+? ?   
> 
> This bug blocks any Fennec Native users trying to sign in on sites that
> implement current BrowserID flow. 
> 
> See bug 730015 and https://github.com/mozilla/browserid/issues/1169 for more
> info.
> 
> Tony

fyi, XUL fennec works fine.  (up to FF11)

Comment 4

[Matt Brubeck (:mbrubeck)]

(In reply to Matt Brubeck (:mbrubeck) from comment #1)
> What does XUL Fennec do?

The answer is: XUL Fennec fails silently with NS_ERROR_FAILURE from nsIDOMJSWindow.open.  (The reason BrowserID was working on XUL Fennec is because they sniffed for the Fennec UA and used a workaround for this bug.)

I think what we need here is an option (perhaps a pref?) to ignore "dialog=yes" in window.open arguments, and just treat open a non-dialog window/tab instead.

Comment 5

[:Margaret Leibovic]

Attachment: patch

So, I spent a lot of time munging through this code to make sure we won't break anything, and I think we're okay with this approach. mbrubeck, if you think this is okay, I'll ask for review from a core reviewer.

It appears the reason this was failing while other ways of opening windows was working is that we were never calling nsIWindowProvider->ProvideWindow here:
http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#624

which leads to determining where we open a new window here:
http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsContentTreeOwner.cpp#876

It looks like we can also miss that path by setting the chrome or modal features on the window, but we shouldn't need to worry about those because only privileged scripts can do that.

Comment 6

[:Margaret Leibovic]

I should also mention that this won't affect dialogs created by the internal openDialog() methods (those flip the aDialog flag instead of relying on the aFeatures parameter).

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 602518 [details] [diff] [review]
patch

Nice.

Comment 8

[Matt Brubeck (:mbrubeck)]

Comment on attachment 602518 [details] [diff] [review]
patch

We should enable the pref for XUL Fennec too.

Comment 9

[:Margaret Leibovic]

Attachment: flip pref for XUL fennec

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 602518 [details] [diff] [review]
patch

Seems it'd make more sense to follow the existing preference naming scheme here, which is to say that the preference name for this new feature should be "dom.disable_window_open_feature.dialog". Does that make sense? Or am I missing something here?

Other than that, this looks good. r=jst with that considered here.

Comment 11

[:Margaret Leibovic]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #10)

> Seems it'd make more sense to follow the existing preference naming scheme
> here, which is to say that the preference name for this new feature should
> be "dom.disable_window_open_feature.dialog". Does that make sense? Or am I
> missing something here?

I decided to name it something different because it doesn't go through the same logic in the NS_CALCULATE_CHROME_FLAG_FOR macro as all the ""dom.disable_window_open_feature." prefs, so the semantics of it are different.

Comment 12

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/032eb69f27de
https://hg.mozilla.org/integration/mozilla-inbound/rev/0db466f032ef

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/032eb69f27de
https://hg.mozilla.org/mozilla-central/rev/0db466f032ef

Comment 14

[Tony Chung [:tchung]]

Verified fix on 03-12-2012 fennec nightly build using testcase from comment 1.

Comment 15

[Martijn Wargers (dead)]

I was hoping/thinking this would fix showModalDialog too, but it doesn't appear to, so I filed bug 735237 for that.