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)
Tracking
()
VERIFIED
FIXED
mozilla1.7alpha
People
(Reporter: mjh-bugs-mozilla-20060801, Assigned: danm.moz)
References
()
Details
Attachments
(1 file, 2 obsolete files)
2.43 KB,
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).
Comment 1•21 years ago
|
||
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....
Reporter | ||
Comment 2•21 years ago
|
||
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
Reporter | ||
Comment 4•21 years ago
|
||
> does your patch work on http://www.cnn.com/ and http://home.netscape.com/
I believe so.
cnn produces:
http://www.cnn.com/cnn_adspaces/adsPopup2.html?0
home.netscape.com produces:
http://wp.netscape.com/ex/shak/home_popups/popup_03.html?num43271708031771940
http://home.netscape.com/_ads/adsPopup2.htm?0
does that match what you'd expect?
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 ;)
Reporter | ||
Comment 6•21 years ago
|
||
> 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.
Reporter | ||
Comment 8•21 years ago
|
||
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. ***
Assignee | ||
Comment 10•21 years ago
|
||
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
Assignee | ||
Comment 11•21 years ago
|
||
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)
Comment 12•21 years ago
|
||
Not a huge deal, but can you pass in nsnull for the charset argument, rather than 0?
Comment 13•21 years ago
|
||
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+
Comment 14•21 years ago
|
||
Cool. No more popups on http://www.pirates.bethsoft.com/flash/fbplus.html from
now on?
Comment 15•21 years ago
|
||
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+
Assignee | ||
Comment 16•21 years ago
|
||
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
Assignee | ||
Comment 17•21 years ago
|
||
Attachment #137273 -
Attachment is obsolete: true
Attachment #137328 -
Flags: superreview?(jst)
Attachment #137328 -
Flags: review?(bz-vacation)
Comment 18•21 years ago
|
||
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 19•21 years ago
|
||
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
Comment 20•21 years ago
|
||
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");
Assignee | ||
Comment 21•21 years ago
|
||
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.
Comment 22•21 years ago
|
||
That might be the case but http://www.cnn.com/US/ opens
/cnn_adspaces/adsPopup2.html?0 and popupWindowURI is still null
Assignee | ||
Comment 23•21 years ago
|
||
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.
Comment 24•21 years ago
|
||
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)?
Assignee | ||
Comment 25•21 years ago
|
||
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.
Comment 26•21 years ago
|
||
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.
Comment 27•21 years ago
|
||
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
Assignee | ||
Comment 28•21 years ago
|
||
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.
Description
•