A little real-world testing shows the design of the popup blocking mechanism is flawed: it doesn't work for popups posed from unload handlers. There's no way to add such a site to the blacklist after the popup because the opening page has been closed (so, no menu) and the popup's opener's URI is unobtainable.

Rather than fetch the opener's location when the time comes to deal with the popup, explicitly save it in the popup window just after it's created. Note I chose to use nsIWebBrowserChrome to accomplish this, so I'm revving it to Chrome2. Ugh. Any better place come to mind? I'm also moving the popup manager abuse check over to the DOM window. This means it's taken care of before the application level, so embeddors need not worry about that; they need only write a pref UI. I'm also updating nsWindowWatcher to listen to release the reference it holds to the nsIWindowCreator service at XPCOM shutdown. Well, it was in my tree and I had to touch this file and I'd rather just check it in than unentwine it from the rest of my patch and check it in later. Am I bad? I'll be sure to mention it in the checkin comments.

Wouldn't it make sense to just block the onunload handler altogether with user_pref("capability.policy.default.Window.onunload", "noAccess"); ? One has to be quite creative in order to come up with legitimate and truly useful uses for that event handler.

unload popups are already covered by the perhaps confusingly named dom.disable_open_during_load. And regardless of whether they deserve special treatment of their own, unload windows are part of the popup problem and shouldn't be immune to popup control.

Comment on attachment 101963 [details] [diff] [review] handle popups from unload handlers >- if (CheckForAbusePoint()) >+ if (CheckForAbusePoint(PR_FALSE)) > return NS_OK; Hmmm, obscure boolean param -- use an enum instead? >@@ -2824,8 +2824,9 @@ > * handler, or running a timeout. The window.open code uses this > * routine to determine wether or not to allow the new window. > */ >+#include "nsIPopupWindowManager.h" > PRBool Ahem. How about a blank line between #include and the function return type? Worse, aren't you making the dom depend on xpfe? Don't do that. Define an interface in the dom/public/... subtree that various embedders can implement. Maybe that just means moving nsIPopupWindowManager.h (.idl?) from xpfe to dom. >-GlobalWindowImpl::CheckForAbusePoint () >+GlobalWindowImpl::CheckForAbusePoint (PRBool aNormalOpen) aNormalOpen is kinda vague -- what do you really mean? Oh, you mean true for window.open, false for alert/confirm/prompt. How about aWindowDotOpen or (better, I think), that enum with the various kinds spelled out? > { > nsCOMPtr<nsIDocShellTreeItem> item(do_QueryInterface(mDocShell)); > >@@ -2843,6 +2844,7 @@ > return PR_FALSE; > > if (!mIsDocumentLoaded || mRunningTimeout) { >+ Ahem^2: don't add gratuitous vertical space here (move this blank line up to just after the include). > PRBool blockOpenOnLoad = PR_FALSE; > prefs->GetBoolPref("dom.disable_open_during_load", &blockOpenOnLoad); > if (blockOpenOnLoad) { >@@ -2852,6 +2854,23 @@ > #endif > return PR_TRUE; > } >+ >+ if (aNormalOpen) { >+ nsCOMPtr<nsIPopupWindowManager> pm(do_GetService(NS_POPUPWINDOWMANAGER_CONTRACTID)); >+ if (pm) { >+ PRUint32 permission; >+ >+ nsCOMPtr<nsIDocument> doc(do_QueryInterface(mDocument)); >+ nsCOMPtr<nsIURI> uri; >+ if (doc) >+ doc->GetDocumentURL(getter_AddRefs(uri)); >+ >+ if (NS_SUCCEEDED(pm->TestPermission(uri, &permission)) && Nit: The usual metaphor-verb is "Check" for "Permission", not "Test". >+ permission == nsIPopupWindowManager::DENY_POPUP) >+ return PR_TRUE; >+ } >+ } >+ > } else { > PRInt32 clickDelay = 0; > prefs->GetIntPref("dom.disable_open_click_delay", &clickDelay); >@@ -2932,7 +2951,7 @@ > * If we're in a commonly abused state (top level script, running a timeout, > * or onload/onunload), and the preference is enabled, prevent window.open(). > */ >- if (CheckForAbusePoint()) { >+ if (CheckForAbusePoint(PR_TRUE)) { > if (name.IsEmpty()) { > return NS_OK; > } [snip] >@@ -607,9 +615,25 @@ > rv = mWindowCreator->CreateChromeWindow(parentChrome, chromeFlags, > getter_AddRefs(newChrome)); > if (newChrome) { >- /* It might be a chrome nsXULWindow, in which case it won't have >- an nsIDOMWindow (primary content shell). But in that case, it'll >- be able to hand over an nsIDocShellTreeItem directly. */ >+ if (aParent) { >+ Without the cluttered-looking C-style comment squatting atop the inner if condition, I see no need for an extra blank line, but you're emporer of this Rome, so who am I to complain? >+ nsCOMPtr<nsIDOMDocument> domDoc; >+ nsCOMPtr<nsIURI> parentURI; >+ >+ aParent->GetDocument(getter_AddRefs(domDoc)); >+ nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDoc)); >+ if (doc) >+ doc->GetDocumentURL(getter_AddRefs(parentURI)); >+ >+ nsCOMPtr<nsIWebBrowserChrome2> newChrome2(do_QueryInterface(newChrome)); >+ if (parentURI && newChrome2) >+ newChrome2->SetContentOpenerURI(parentURI); This is the kind of XPCOM code I can't stand. Is it possible for the doc QI to fail? How about the newChrome2 one? If all these really can fail, then it still seems better to me, although more indented, to localize variables to minimal-length blocks: + nsCOMPtr<nsIDOMDocument> domDoc; + aParent->GetDocument(getter_AddRefs(domDoc)); + nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDoc)); + if (doc) { + nsCOMPtr<nsIURI> parentURI; + doc->GetDocumentURL(getter_AddRefs(parentURI)); + + if (parentURI) { + nsCOMPtr<nsIWebBrowserChrome2> newChrome2(do_QueryInterface(newChrome)); + if (newChrome2) + newChrome2->SetContentOpenerURI(parentURI); + } + } But I'm really picking a nit here. In the successful case, you don't do any extra tests, and the code is less indented. In the failure case, who cares about extra tests of default-init'ed-to-null nsCOMPtrs. So never mind me, except for the legit question about what can be proven never to fail here (the doc QI?). >+NS_IMETHODIMP >+nsWindowWatcher::Observe(nsISupports *aSubject, const char *aTopic, >+ const PRUnichar *aData) >+{ >+ if (nsCRT::strcmp(aTopic, sXPCOMShutdownTopic) == 0) { >+ // release the services we're holding >+ mOS->RemoveObserver(this, sXPCOMShutdownTopic); >+ mOS = 0; >+ mWindowCreator = 0; Nits: nsnull? >+++ xpfe/appshell/src/nsChromeTreeOwner.cpp 7 Oct 2002 08:55:43 -0000 >@@ -91,6 +91,8 @@ > return mXULWindow->GetInterface(aIID, aSink); > if(aIID.Equals(NS_GET_IID(nsIWebBrowserChrome))) > return mXULWindow->GetInterface(aIID, aSink); >+ if(aIID.Equals(NS_GET_IID(nsIWebBrowserChrome2))) >+ return mXULWindow->GetInterface(aIID, aSink); > if (aIID.Equals(NS_GET_IID(nsIEmbeddingSiteWindow))) > return mXULWindow->GetInterface(aIID, aSink); > if (aIID.Equals(NS_GET_IID(nsIEmbeddingSiteWindow2))) >Index: xpfe/appshell/src/nsContentTreeOwner.cpp Nit: which if-condition spacing style should prevail here? >+++ xpfe/appshell/src/nsXULWindow.cpp 7 Oct 2002 08:55:54 -0000 >@@ -153,6 +153,10 @@ > NS_SUCCEEDED(EnsureContentTreeOwner()) && > NS_SUCCEEDED(mContentTreeOwner->QueryInterface(aIID, aSink))) > return NS_OK; >+ if (aIID.Equals(NS_GET_IID(nsIWebBrowserChrome2)) && >+ NS_SUCCEEDED(EnsureContentTreeOwner()) && >+ NS_SUCCEEDED(mContentTreeOwner->QueryInterface(aIID, aSink))) Nit: Shouldn't the overlong condition's non-initial && clauses line up with the first clause? >+ return NS_OK; > > if (aIID.Equals(NS_GET_IID(nsIEmbeddingSiteWindow)) && > NS_SUCCEEDED(EnsureContentTreeOwner()) && >+ var treeOwner = window >+ .QueryInterface(CI.nsIInterfaceRequestor) >+ .getInterface(CI.nsIWebNavigation) >+ .QueryInterface(CI.nsIDocShellTreeItem).treeOwner >+ .QueryInterface(CI.nsIInterfaceRequestor); >+ var xulwin = treeOwner.getInterface(CI.nsIXULWindow); >+ var chrome = treeOwner.getInterface(CI.nsIWebBrowserChrome2); This mouthful repeats again, and you've already had to change it once -- I'm just wondering whether a getTreeOwner JS helper method on window wouldn't be worth it. >[scriptable, uuid(339AF910-0AAA-4420-B805-7D1679EA4E46)] >interface nsIWebBrowserChrome2 : nsIWebBrowserChrome >{ > /** > * chrome may be asked to hold the URI of the content window that opened it. > */ > readonly attribute nsIURI contentOpenerURI; > [noscript] > void SetContentOpenerURI(in nsIURI aURI); >}; > Sorry, no caffeine yet. Suppose an embedding app doesn't implement this new interface -- will popup blocking work, but simply fail in the onunload case? Or will all popup blocking fail? I.e., do we fall back on opener.content.location if this interface isn't impl'd? Should we? /be

>>+ if (CheckForAbusePoint(PR_FALSE)) >Hmmm, obscure boolean param -- use an enum instead? Oh for petesake. How readable does a tiny private method have to be? Alright, I've made it an enum. >Ahem. How about a blank line between #include and the function return type? >Worse, aren't you making the dom depend on xpfe? Don't do that. Define an >interface in the dom/public/... subtree that various embedders can implement. >Maybe that just means moving nsIPopupWindowManager.h (.idl?) from xpfe to dom. Heh. Heh heh heh. Notice no change to Makefile.in. Yes thanks for noticing I should move the #include up to the top with the rest of them. Done. However, the DOM already has a compile-time dependency on xpfe/appshell. I've added nothing new there. If you insist, I can move nsIPopupWindowManager.idl to dom/public/base (?). Neither place is a good home for that idl. extensions/cookie wants its back-end to be factored into a non-extension. Until that happens, it's all pointless shuffling. >>+ if (NS_SUCCEEDED(pm->TestPermission(uri, &permission)) && >Nit: The usual metaphor-verb is "Check" for "Permission", not "Test". What NOW? OK, done. >This is the kind of XPCOM code I can't stand. Is it possible for the doc QI to >fail? How about the newChrome2 one? If all these really can fail, then it >still seems better to me, although more indented, to localize variables to >minimal-length blocks: Fine. And whether doc can fail; hard to say. This code runs in embedded apps too. It's probably not the first place that'll fail if an embedded app doesn't support both interfaces but a crash check seems cheap. And yes absolutely, I expect newChrome2 will usually fail. If either fails the app just loses some popup management functionality. >>+ mWindowCreator = 0; >Nits: nsnull? I'm with Scott. nsnull is dopey. Personally I find it less readable, though one gets accustomed to anything in time. Except sharp sticks, maybe. >+ if(aIID.Equals(NS_GET_IID(nsIWebBrowserChrome2))) >+ return mXULWindow->GetInterface(aIID, aSink); >Nit: which if-condition spacing style should prevail here? done. >>+ NS_SUCCEEDED(EnsureContentTreeOwner()) && >>+ NS_SUCCEEDED(mContentTreeOwner->QueryInterface(aIID, aSink))) >Nit: Shouldn't the overlong condition's non-initial && clauses line up with the >first clause? Dude you are so getting a "brendan made me do it" comment in this file. >>+ var xulwin = treeOwner.getInterface(CI.nsIXULWindow); >>+ var chrome = treeOwner.getInterface(CI.nsIWebBrowserChrome2); >This mouthful repeats again, and you've already had to change it once -- I'm >just wondering whether a getTreeOwner JS helper method on window wouldn't be >worth it. Maybe. Let me just say that if anyone else tries to do this ugly stuff, make them stop. Convenience method! >>[scriptable, uuid(339AF910-0AAA-4420-B805-7D1679EA4E46)] >>interface nsIWebBrowserChrome2 : nsIWebBrowserChrome >Sorry, no caffeine yet. Suppose an embedding app doesn't implement this new >interface -- will popup blocking work, but simply fail in the onunload case? >Or will all popup blocking fail? I.e., do we fall back on >opener.content.location if this interface isn't impl'd? Should we? If an embedding app wants popup blocking, they should implement the interface. If they don't, blocking isn't disabled. But they may have trouble implementing some of their UI. At least it won't crash. Null checks, you know. So implement the interface.

Can you make this fire a custom DOMPopupBlocked event when a popup is blocked? That would be extremely handy for XUL UI that wants to do something when the popup is blocked (e.g., display something in the status bar), and it's like 3 lines of code to do.

danm, do you have a sample url where this can be tested? thanks!

Cha! None that I'm willing to admit to. Snort. OK, I just added a site to this bug's URL field. It stresses popup management fairly seriously. Just go there and poke around. Har. I kill myself. I'd like to point out that my interest in this site lies solely in the richness of its popup management exercise. And I was only interested in the text. Seriously, it's easy enough to make your own test suite with unload popups. These guys are however pulling at least two more interesting tricks that require DNS cooperation and/or multiple servers to duplicate: they use random subdomains, and frames from different domains. Very creative. All problematic for our blacklist blocking code without the patch in this bug. A whitelist scheme of course would have no problems with any of this, other than you'd never get any notification of a blocked unload popup.

Moving an observation over from bug 167559. Mozilla currently does not block most alerts, even from sites for which popup blocking is active. However it does block alerts posed in the <script> tag. E.g., <html><head><script> alert('boo') </script> That's yet another problem that a (slightly modified) version of the patch in this bug will fix, as well as two other problems outlined in comment 8.

nominating to alleviate the unfortunate state of affairs described in bug 167559 comment 32. Doron says that not fixing bug 167559 will make popup blocking a _major_ evang/compat issue on many sites... if we don't fix it, we may as well just not ship the thing.

see also bug 174775

nominating for 1.0.2

Changing qa contact to sarah

adding dveditz to cc list

after a bit of discussion, updating qa contact to nobody for the interim. if there's anyone who has the cycles to QA this, however, do let me know!

So this will fix alert()'s being blocked, right?

Doron, thanks for spamming every possible bug with this observation. We see you. To answer your question, see comment 9. Popup blocking has been backed out. This bug is now inapplicable, which is a kind of fix. However it may return when popup blocking is reinstated, so I'm leaving it open as a reminder.

*** Bug 180245 has been marked as a duplicate of this bug. ***

confirm can be used as an exploit. consider the following scenario. var x = confirm("Do u want to leave this site ") if (x == true) document.location.href="http://www.google.com"; else document.location.href="http://www.google.com";

In what way is that an exploit?

it may restrict you need to leave the site, or send you to the site which he desires. So it is indirectly hurting the pop-up feature.

It can do that anyway, even without the prompt()

This applies only to blacklist popup management. Since Mozilla now supports only whitelists (bug 195924), this bug is become moot.