Closed Bug 1239885 Opened 8 years ago Closed 8 years ago

Popup blocker notifications don't work on "data:" pages when using window.open() with no URI as arguments

Categories

(Toolkit :: UI Widgets, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- fixed

People

(Reporter: arni2033, Assigned: Gijs)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20160112030227
STR:
1. Open this "data:" url or click URL in the form above
> data:text/html,<script>addEventListener('click',function(){console.log('click');window.open()})</script>
2.A) Middle-click on the page content (white area)
2.B) Right-click on the page content

Result:       Nothing
Expectations: "Blocked popup" notification should appear

This never worked in e10s mode;
It was broken in non-e10s between 2014-03-21 and 2014-03-22 by bug 933462. Pushlog:
> https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c148f0b0c8b4&tochange=72c2f9ebf4bc
User Agent 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID 	20160126030244

Reproduced on Nightly 47.
Component: Untriaged → Event Handling
Component: Event Handling → General
Product: Core → Firefox
My guess is that some code in FF code is using node.ownerDocument.documentURI or some such, and not
node.nodePrincipal.
(In reply to Olli Pettay [:smaug] from comment #2)
> My guess is that some code in FF code is using
> node.ownerDocument.documentURI or some such, and not
> node.nodePrincipal.

toolkit's browser-content.js is listening for the onPopupBlocked event and throws:

TypeError: ev.popupWindowURI is null

when trying to access the URI for the popup window.

If I change the URI from comment #0 to:

data:text/html,<script>addEventListener('click',function(){console.log('click');window.open("about:blank")})</script>

then it works fine.
Component: General → XUL Widgets
Product: Firefox → Toolkit
Summary: Popup blocker notifications don't work on "data:" pages → Popup blocker notifications don't work on "data:" pages when using window.open() with no URI as arguments
I think the event API is a bit silly here, but at least it should have access to the Window object (event.requestingWindow) on which .open() is called and it can get the right requesting URL from that.
And then what popupWindowURI is is less interesting. empty string or null could default to about:blank I guess.
(In reply to Olli Pettay [:smaug] from comment #4)
> I think the event API is a bit silly here, but at least it should have
> access to the Window object (event.requestingWindow) on which .open() is
> called and it can get the right requesting URL from that.

It looks like it already does that.

> And then what popupWindowURI is is less interesting.

Well, we need it to open the popup if the user elects to do that, and for that we need the URI.

> empty string or null could default to about:blank I guess.

Are you saying browser-content.js should be taught to generate about:blank as a URI for a null popupWindowURI, or that we should change nsGlobalWindow to use about:blank in this case, so the popupWindowURI is never null? (ni for this so I know where we expect to fix this :-) )


To be clear, in pretty much any case where a page opens window.open() with no URI, it'll want to use the return value of the window.open() call to actually manipulate that window (usually synchronously), either by inserting content into about:blank or by navigating it somewhere else. I'm assuming that that return value is null in cases where we block popups (that's certainly the case in this testcase) and so that'll break page script no matter what we do.
Flags: needinfo?(bugs)
I wouldn't change nsGlobalWindow since it just passes whatever url was given to window.open.
But what I meant that toolkit could handle null similarly to about:blank. both those are most probably used so that page script will then populate the content.
Flags: needinfo?(bugs)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8731322 [details]
MozReview Request: Bug 1239885 - fix popup blocking brokenness when no URI is passed to window.open, r?felipe

https://reviewboard.mozilla.org/r/40495/#review37189
Attachment #8731322 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/1c5926c2e71f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Looks like this should work in 46 (with e10s disabled). The non-e10s regression was only in nightly 48.
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11)
> Looks like this should work in 46 (with e10s disabled). The non-e10s
> regression was only in nightly 48.

No, this was broken in non-e10s by bug 933462, according to comment #0 . However, given the fact that this is an edgecase with little practical value, I don't think this needs uplift.
You need to log in before you can comment on or make changes to this bug.