Today my dad complained to me that his webmail wasn't working right in Firefox. I looked into it, and he's basically seeing bug 102481. As described in that bug, Outlook Web Access does the following actions when the reply button is clicked: 1) Load a new page 2) The new page's onload handler calls window.open() to open a new window and then closes the old window. The problem, of course, is that the new window is blocked by the popup blocker and the old window is closed so the yellow bar that says a popup was blocked never appears. Now I fixed Dad's issue by adding the site to the popup whitelist, but that involved knowing what the problem is (in which I was helped by recalling the existence of bug 102481), going into preferences, etc. The real issue is that the user never got any UI indicating that a popup was blocked. To handle this case better, I propose that we ignore calls to window.close() that happen "immediately" after a failed popup-opening attempt. That way the user will see the yellow bar, and either add the site to the popup whitelist (which will make it work) or leave it as-is (in which case it will be arguably less broken than it is now). The simplest implementation would be to set some sort of flag on the nsJSContext when a popup is blocked and clear it in ScriptEvaluated. If window.close is called from non-chrome code while the flag is set, ignore the call. If we use a counter instead of just a boolean flag (and in ScriptEvaluated reset to the value the counter had when we started the script execution), we can effectively prevent window.close until the script that tried opening the popup actually finishes evaluating; I think this would be the optimal way to do this. Thoughts on the idea? Any obvious problems with it? Worth trying to do for 1.8/1.1?

This test case is a little different from bz's I think, but illustrates a related issue. openclose has a js link which opens open1. open1 opens open2 which calls window.opener.close() to close open1. run on my local web server, open2 is blocked, no yellow info bar is displayed in open1 although the X is displayed in the right hand corner of the status bar. run from bugzilla, we do display the yellow info bar in open1 (at least for me). IE shows their info bar when open2 is blocked. This seems like a race condition.

If I change open1 to call window.close() immediately after the window.open call, then both Firefox and IE will block open2, then close open1 with no indication that a popup was blocked.

Right. The behavior described in comment 4 is exactly the behavior I propose we change.

Two examples of where this happens are Outlook Web Access (bug 102481) and NatWest bank (bug 296645).

I think this may be related to this bug, let me know if it's a separate issue entirely. I've tested in Firefox 1.5rc1. When the prefence to "Force links that open new windows to open in: the same tab/window as the link" is enabled, following a link that attempts to open a new window will load the document in the same tab, and then any attempt on that page to close the window, whether by clicking a link or, even worse, in the onload event handler, the same tab will close even though previous script never actually opened the window; it just reused it. Testcase: http://lachy.id.au/dev/2005/11/window-open-close-test

> let me know if it's a separate issue entirely It is. It's even filed, last I checked (for windows, not tabs, but same deal).

(In reply to comment #8) > > let me know if it's a separate issue entirely > > It is. It's even filed, last I checked (for windows, not tabs, but same deal). Ok, I couldn't find that specific bug, but I'll believe you and won't bother filing another. Thanks.

Bug 266371, I believe.

Yesterday I filed bug 321027 that seems to be related to this one. Can anybody check that up? There is also another bug 312184 that seems related.

> Yesterday I filed bug 321027 that seems to be related to this one. It's not. > There is also another bug 312184 that seems related. That's the same as bug 321027 and I think both are duplicates of an existing bug, but not of this one. I'd suggest looking for that original bug...

This fix counts blocked popups and does not close the window (via javascript) if the counter is > 0. The counter is a property of the global object of the js context. I did not dare to add the counter as a member of nsJSContext. Would this be a better way?

You're decrementing the popup counter for non-popup windows, no? And incrementing it for popups that are blocked? I'm not sure that makes sense. I also don't think it makes sense to block window.close() because sometime in the past a long time ago a popup was blocked. Finally, setting properties on the window object as you do and relying on them is bad -- pages can read and write those. I really do think the right place to keep track of this is by setting a flag when a popup is blocked and clearing that flag from a script termination function that you set at the same time...

This patch sets a flag (nsPIDOMWindow::mBlockScriptedClosingFlag) on a window (nsGlobalWindow) everytime a open window call is blocked. The method nsGlobalWindow::Close refuses to close the window if this flag is set. The flag is cleared on script termination.

You want the currently-running context, not this window's context. See what nsGlobalWindow::Close does. And you probably just want to put this stuff on nsGlobalWindow and cast as needed (again, like CloseWindow does) instead of modifying nsPIDOMWindow. Other than that, this looks pretty good; I assume it works and all, right?

All right, I'm using the current js context now. The flag has been moved to nsGlobalWindow - nsPIDOMWindow remains untouched. And I'd say it works. :) I've tested the patch with the testcases above and some slightly modified versions. The window that opens the popup is not closed and I am notified with a little icon in the status bar. But: the yellow info bar saying "Firefox prevented this site from opening a popup window" does not show up. But that's another bug (feature?), isn't it?

sorry, this is the right file...

Hmm... it's hard to tell. We do still fire the event that normally makes the yellow infobar appear, right? Why would it not show up?

(In reply to comment #19) Ok the yellow bar issue becomes clearer now: The yellow bar does not appear when I load the test files above from a local directory. The bar _does_ appear after I press F5 in the window that tries to open a popup (open1.html). Using the links above (nonlocal) I see that bar at the first attempt. I can reproduce this behaviour with Firefox 1.5.0.6 (unpatched of course), so it's not my fault. ;-)

(In reply to comment #20) > I can reproduce this behaviour with Firefox 1.5.0.6 (unpatched of course), so > it's not my fault. ;-) I can reproduce this with 1.5.0.x builds, too, but not current 1.8 branch or trunk builds. Perhaps the issue was fixed, or perhaps there's a timing difference between the builds I was testing (all with new profiles). Either way, if you can reproduce in a current build, go ahead and file a bug and CC me. I don't think it should block development here. I also wanted to try your patch, but noticed that it no longer applies cleanly to the trunk.

(In reply to comment #21) > I also wanted to try your patch, but noticed that it no longer applies cleanly > to the trunk. Ah indeed. Sorry, I'll merge it to the trunk.

Joerg, why is the aCurrentCallerContext argument needed? By the way, let me know when the patch is ready for review (or just request review on the patch).

(In reply to comment #24) > Joerg, why is the aCurrentCallerContext argument needed? I just didn't want to copy and paste the code for retrieving the native caller context from Open (line 4325-4339) to minimize my changes to OpenInternal. That's the only reason. BTW I can't reproduce the yellow info bar problem with the current trunk. I accidently used a 1.5.0.x branch for the former patches. Sorry for that. <:-(

I guess what I meant was why we're bothering comparing aCurrentCallerContext to anything. Why not just always set the termination func when !aCalledNoScript?

(In reply to comment #21) > (In reply to comment #20) > > I can reproduce this behaviour with Firefox 1.5.0.6 (unpatched of course), so > > it's not my fault. ;-) > > I can reproduce this with 1.5.0.x builds, too, but not current 1.8 branch or > trunk builds. Perhaps the issue was fixed Ah-ha, I knew this sounded familiar: I fixed it in bug 305912.

replaced comparision "current contex == caller context" with "window context == caller context"

Comment on attachment 235289 [details] [diff] [review] fix #4 >Index: nsGlobalWindow.cpp >+ // A scripts' popup has been blocked and we don't want "A script's" >+ // the window to be closed directly after this event. >+ // So the user can see that there was a blocked popup. "event, so the user" >+nsGlobalWindow::CloseBlockScriptTerminationFunc(nsISupports *aRef) >+ nsGlobalWindow* pwin = NS_STATIC_CAST(nsGlobalWindow*, NS_STATIC_CAST(nsPIDOMWindow*, aRef)); Can you keep this under 80 chars wide by inserting line-breaks as needed, please? > nsGlobalWindow::OpenInternal(const nsAString& aUrl, const nsAString& aName, This should probably have: NS_PRECONDITION(!aJSCallerContext || !aCalledNoScript, "Shouldn't have caller context when called noscript"); right after the FORWARD_TO_OUTER part. >+ if (!aCalledNoScript && aJSCallerContext) And then there's no need for the !aCalledNoScript check here. >+ if (mContext == GetScriptContextFromJSContext(aJSCallerContext)) { // window context == caller context? I'd remove that comment and replace it with something more like: // If script in some other window is doing a window.open on us and // it's being blocked, then it's OK to close us afterwards, probably. // But if we're doing a window.open on ourselves and block the popup, // prevent this window from closing until after this script terminates // so that whatever popup blocker UI the app has will be visible. That is, document why things are done, not what's done. >Index: nsGlobalWindow.h >+ JSContext *aJSCallerContext, Please document this argument (and its relationship to other arguments, like aCalledNoScript, in the big comment before this method declaration. r=bzbarsky with those nits picked. jst, could you sr, please?

Comment on attachment 235289 [details] [diff] [review] fix #4 What bz said. sr=jst

minor changes, see bz's comment

Patch checked in to trunk. Joerg, thanks for doing this, and again apologies for my earlier misunderstanding of the patch.