Closed Bug 212460 Opened 21 years ago Closed 21 years ago

DOMPopupBlocked event: popupwindowURI is null if window.open() called with a relative url

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.7alpha

People

(Reporter: mjh-bugs-mozilla-20060801, Assigned: danm.moz)

References

(
URL
)

Details

Attachments

(1 file, 2 obsolete files)

construct popup URI the same way it would have been opened
(deleted), patch
bzbarsky
: review+
jst
: superreview+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (compatible; Konqueror/3.1; Linux) Build Identifier: Mozilla 1.4 The FirePopupBlockedEvent calls in GlobalWindowImpl::Open create their URIs from the given url using nsIIOService->NewURI. However if that url does not start with a protocol id, you get null, not the URI in question, even though the link itself works (if you disable the block). The url needs to be run through something like nsWindowWatcher::URIfromURL (which is a private member function in a seperate library and thus not easily accessible or I'd have written a patch). Reproducible: Always Steps to Reproduce: 1. install an event watcher on DOMPopupBlocked 2. Visit any site with popups that use relative links (http://games.yahoo.com is the one I've been testing against). 3. examine popupWindowURI in the event handler. Actual Results: popupWindowURI is null, even if url in GlobalWindowImpl::Open is not. (games.yahoo.com prompts urls such as /games/ante?room=literati_4&prof_id=chat_pf_1) Expected Results: The actual url (or an absolute url that is equivalent).
Well, the fix is pretty simple -- pass in the originating URI as a base URI. Problem is that the originating URI this code gets is pretty bogus....
Attached patch baseURI patch (obsolete) (deleted) — — Splinter Review
At least some of the time the refering URI seems sane (my test cases were fine anyway). So this patch passes the base URI in.
Confirming on WinNT4: OS -> ALL Malcolm, does your patch work on http://www.cnn.com/ and http://home.netscape.com/
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Yes, but can you please attach a working testcase, or two, to this bug report? This way everybody can test it here and see that it doesn't work without your patch. They can simply test the patch after they applied it to their own tree. I think that would help us get this thing included to the trunk one day. "Problem is that the originating URI this code gets is pretty bogus...." Ttrue, just visit http://dictionary.reference.com/search?q=bogus and see for yourself. The url I get is: javascript:"<head><title>.... I even wonder if that URL is valid but it seems to work ;)
> Yes, but can you please attach a working testcase, or two, to this bug report? What are you looking for? Url's that break, or the javascript to illustrate the breakage, or something else?
Just a small working testcase that triggers the bug. It can also be used after applying your patch, and for future reference, in case someone breaks it again.
The following should be sufficient. a.html: <html><body> <a href="#link" onClick='window.open("/b.html");'>a link</a> </body></html> b.html: <html><body>opened window</body></html> With blocking off, you would get the second window. With it on, you wouldn't and a popup blocked event would be sent, which you can check with the following javascript: addEventListener("DOMPopupBlocked", on_blocked, true); function on_blocked(event) { var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService( ); promptService = promptService.QueryInterface (Components.interfaces.nsIPromptService); var pbe = event.QueryInterface(Components.interfaces.nsIDOMPopupBlockedEvent); promptService.alert(window, "", "blocked - " + pbe.popupWindowURI.spec); } Is that what you needed?
*** Bug 176079 has been marked as a duplicate of this bug. ***
Taking from saari. The bug is real, it's my code, the patch is good (though not perfect), saari is no longer paying attention, let's get it fixed.
Assignee: saari → danm-moz
Target Milestone: --- → mozilla1.7alpha
Malcolm's original patch was right on, but tripped up over documents with base URIs (<html><head><base href="xxx">...) This version should do it.
Attachment #128454 - Attachment is obsolete: true
Attachment #137273 - Flags: superreview?(jst)
Attachment #137273 - Flags: review?(bz-vacation)
Not a huge deal, but can you pass in nsnull for the charset argument, rather than 0?
Comment on attachment 137273 [details] [diff] [review] construct blocked popup event's popup URI using document's base URL + nsCOMPtr<nsIURI> baseURL; + nsCOMPtr<nsIDocument> doc(do_QueryInterface(mDocument)); + if (doc) + baseURL = doc->GetBaseURL(); In the new at-least-somewhat deCOMtaminated world we now live in, you can leave baseURL a raw pointer and save a few bytes of code. sr=jst
Attachment #137273 - Flags: superreview?(jst) → superreview+
Cool. No more popups on http://www.pirates.bethsoft.com/flash/fbplus.html from now on?
Comment on attachment 137273 [details] [diff] [review] construct blocked popup event's popup URI using document's base URL r=bzbarsky if you fix jst's nit and assuming that you tested that this gives the right results when script in window A calls window.open() on window B with a relative URL (that is, that we use the same base (that of window B is used in this patch) for resolving the relative URL both here and wherever we actually open the window).
Attachment #137273 - Flags: review?(bz-vacation) → review+
Johnny: Cool, I didn't know whether that was considered safe. Christopher: Augh! Stop by some day for my rant on Coding Styles of the Self-Proclaimed Priesthood. In short, I hate nsnull and I'm sorry you like it. HJ: I've seen bug 176079 comment 12 and any time someone says "do some debugging" with a smiley it makes me extra careful. But still, I don't see the connection. This code is never triggered in that case. See bug 176079 comment 14. Boris: I'm finding so many bugs as I poke around in this I don't know where to begin. Alright, focus... You're right, the window opening code itself prefers a context from the script call stack. When opening across windows (script in window A that reads B.open(<url>)), tell me, is it right that we use resolve the url relative to A? Possibly. If yes, then how should we resolve B.dfunc(url), where B.dfunc is a local function that calls window.open? What about B.tfunc(url), where tfunc calls dfunc on a timer? I think our behaviour in these cases is inconsistent. I think IE's behaviour is also goofy, though arguably more consistent than Mozilla's. And Mozilla and IE don't always agree. I think I've discovered seamy underbelly too esoteric to find anyone else's footprints before me. Anyone but a popup spammer. Oh and I also found a layout bug with iframes and you can fool the popup detector by opening a window from a timeout in a different window and the plugin problem in bug 176079 seems difficult. Woe.
Status: NEW → ASSIGNED
Attachment #137273 - Attachment is obsolete: true
Attachment #137328 - Flags: superreview?(jst)
Attachment #137328 - Flags: review?(bz-vacation)
Comment on attachment 137328 [details] [diff] [review] construct popup URI the same way it would have been opened There we go. r=bzbarsky
Attachment #137328 - Flags: review?(bz-vacation) → review+
Comment on attachment 137328 [details] [diff] [review] construct popup URI the same way it would have been opened sr=jst
Attachment #137328 - Flags: superreview?(jst) → superreview+
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Hmm, I don't know why but 'popupwindowURI' is still null on some sites like CNN. I'm using mozilla build 2004010208 on WinXP. This should work in this build? This is what I use in 'onPopupBlocked' if (!popupwindowURI) alert("popupwindowURI == null");
Be more specific, please. In my testing, CNN always attempts to open a popup as soon as one navigates to a second page on the site. Go to http://www.cnn.com/ and click the "World" link in the upper left, get a popup. I'm getting a full, absolute URL when I do that.
That might be the case but http://www.cnn.com/US/ opens /cnn_adspaces/adsPopup2.html?0 and popupWindowURI is still null
Are you referring to the PopupWindow event (the one fired when a popup is opened), rather than the DOMPopupBlocked event (the one fired when a popup is refused)? The code in this patch doesn't address that event at all. The popupwindow event doesn't have a popupWindowURI property. Well, not completely true. All DOM events in Mozilla report properties from event types that they aren't. So the popupwindow event reports a popupWindowURI property, though it's null. So do mousedown events. Both types also sort of contain anomalous key event properties. This is perhaps a mistake, but that's how it works. The popupwindow event was never intended to include a popupWindowURI property. It's used in only one place in Mozilla code, and there it's a simple notification. The only property referenced is its target. I suppose the popupwindow event could reasonably include those URI properties. If that's what you want, please file a new enhancement bug. Or maybe I've just misunderstood completely. However I'm not seeing a problem with the DOMPopupBlocked event in recent builds. With a dump (it's a debug build) in onPopupBlocked of navigator.js, I get this: open browser directly to http://www.cnn.com/US/ - no popup attempted open browser to http://www.cnn.com/ - no popup attempted click U.S. link, taking me to http://www.cnn.com/US/ - aEvent.requestingWindowURI http://www.cnn.com/US/ - aEvent.popupWindowURI http://www.cnn.com/virtual/editions/europe/ 2000/roof/change.pop/frameset.exclude.html I attempted four links to other summary pages and articles, including one return to /US/. Each gave me a popup event with a different requesting URI but the same popup URI.
Dan, like aI said in comment 20, especially: <quote> This is what I use in 'onPopupBlocked' if (!popupwindowURI) alert("popupwindowURI == null"); </quote> so I am using DOMPopupBlocked, right? Also what platform are you using? Because this might be a Win32 specific bug, is that a valid assumption? Now, here's the code I use: /**** * We need to get notified of blocked popup windows so we replace the * original eventlistener for 'DOMPopupBlocked' with our (this) event * listener and call 'onPopupBlocked' (original function) when we're done. */ addEventListener("DOMPopupBlocked", mzOnPopupBlocked, false); removeEventListener("DOMPopupBlocked", onPopupBlocked, false); function mzOnPopupBlocked(aEvent) { var popupWindowURI = aEvent.popupWindowURI; if (popupWindowURI) { // do good work here } else { alert("Warning: Mozilla failed to initialize 'popupWindowURI'") } onPopupBlocked(aEvent); // call mozilla's 'eventlistener' Oh, and what are you using (it might help me to verify this problem)?
I'm just doing this: Index: xpfe/browser/resources/content/navigator.js =================================================================== RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/navigator.js,v retrieving revision 1.521 diff -u -r1.521 navigator.js --- xpfe/browser/resources/content/navigator.js 3 Dec 2003 21:41:34 -0000 1.521 +++ xpfe/browser/resources/content/navigator.js 8 Jan 2004 02:06:55 -0000 @@ -2118,2 +2118,6 @@ function onPopupBlocked(aEvent) { +dump("********** onPopupBlocked\n"); +dump("requestor "+aEvent.requestingWindowURI.spec+"\n"); +dump("popup "+aEvent.popupWindowURI.spec+"\n"); +dump("**********\n"); var playSound = pref.getBoolPref("privacy.popups.sound_enabled"); I never get a null warning; it just always prints the URI. I too am on WinXP, and my build's from 20041229. I could try updating, I s'pose. PS there's no need to +removeEventListener("DOMPopupBlocked", onPopupBlocked, false); unless you need to suppress normal processing. With two event listeners, it'll just call both (the order in which it calls them is unknown). I suggest you try what I tried, which is minimally invasive. If that works, it should be easy to track down what extra invasion causes the problem.
function mzOnPopupBlocked(aEvent) { dump("\n********** onPopupBlocked\n"); dump("requestor "+aEvent.requestingWindowURI.spec+"\n"); dump("popup "+aEvent.popupWindowURI.spec+"\n"); dump("**********\n"); >> "Error: aEvent.popupWindowURI has no properties" in JS console I use 0 for "dom.disable_open_click_delay" Can that be the cause of this bug? Also note that I'm still using mozilla build 2004010208, again, it should work in that version? "...unless you need to suppress normal processing..." Correct.
Ok, I installed a new mozilla nightly and now it works, without a change. VERIFIED FIXED for me, thanks for all the help Dan.
Status: RESOLVED → VERIFIED
Oh. Cripes. All that trouble, comments 20-27, is because the patch was checked in on 20040102, and it wouldn't have showed up in builds until the following day. Snicker. (And obviously my build isn't from 20041229.) Well, glad to hear it's working for you now.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: