User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release) Build ID: 20140715214327 Steps to reproduce: A website created an ad window displaying this URL: http://healthy-consumer.com/news/skincare-secrets-exposed/?&c1=323exitexitexitexitexitexitexitexitexitexitexitexitexitexitexitexitexitexit&c2=19801&c3=#OFFER Actual results: Clicking the close button in the window frame or trying to close the window using normal shortcuts results in a dialog --- presumaly the onbeforeunload confirmation dialog --- flickering into existence briefly, then disappearing. The location changes to the above, but with another "exit" in the URL. Expected results: The window should close, possibly after prompting. The page should not be able to programatically dismiss the prompt dialog.

When this happens, Browser Console shows: NS_ERROR_NOT_AVAILABLE: prompt aborted by user. Bugs mentioning that are mostly in "Toolkit :: Notifications and Alerts". After that, closing just works.

Tim, could you take a look? In particular, is this an issue in core, or in the beforeunload prompt code?

Looks like something regressed bug 560767, resp. bug 956524? The test case in there is still working and I can't close the tab. The page given here is navigating via |location.href = ...| in onbeforeunload. They're not installing a listener on the new page that's why we can luckily close it at all.

I'm suspecting this is a regression of bug 616853.

Yeah, looks like the new per-tab-modal dialog is cancelled when location.href is called. The truly-modal dialog we had before stayed until the user made a choice.

Not sure which component this actually belongs to. How could we prevent scripts from cancelling the tab-modal dialog?

Isn't the real issue here that we're treating the cancellation via navigation as the same as cancellation via user action? For beforeunload dialogs triggered by navigations, that makes some sense. But for window closing, it does not.

Do we have a testcase? The URL in comment #0 is dead, and someone just reported essentially the same thing: bug 1060349 - but again, without a testcase that breaks for me...

data:text/html,<script>window.onbeforeunload=function(e){e.returnValue="?"; location="http://mozilla.org/"}</script> This WFM but you need to wait for mozilla.org to load. The second try will be much faster when the site is cached. If you're quick enough to hit the dialog we'll close the tab. Otherwise if the page loads before the dialog will be dismissed.

What happens internally seems to be the following: - we create a dialog for the first, user-triggered, unload. PermitUnloadInternal is called: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp?mark=1086-1089#1077 and we create a dialog and spin the event loop, because that's what happens for tab-modal dialogs. - the page href setter triggers another unload. PermitUnloadInternal gets called again, but returns early (see the marked lines above) because we're already in an unload prompt *and says "yes, go and unload the page"*. That then destroys the dialog, and we resume at the initial permitunloadinternal call, hitting the abort here: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#1219 bz's code comment there was on the money, because guess what, now we cancel the original unload (probably wise, because we already unloaded the document in question...) which unfortunately also cancels the original tab- or window close action. :-( Considering that the second unload request works here and will fire an unload event handler, I wonder if we could workaround on the frontend side, to complete the tab/window close action (which, if cued by the unload event, should go ahead because the beforeunload handler will have been killed off, without a new document having been loaded to set up a new one - hopefully? That feels really hacky, though. Alternatively, we could add a parameter to permitunloadinternal, and a different contentviewer endpoint that calls permitunload with the other param set, to ask for permission to unload - for the specific purpose of closing the window. Both closing a tab and closing a window "manually" call permitUnload, and if we switch them to this API, and then make permitunload, when called with this parameter, treat the dialog abort "specially" and return NS_OK and 'true' for the abort case, that would at least allow the user to close the window/tab Neither of these, however, address the fact that with the testcase I attached, if you input "mozilla.org" in the location bar and hit enter, you stay on the same page. I don't know how we'd fix that. Boris, is there a simpler solution here that I'm just missing?

So wait. I'm a bit confused. During the beforeunload event itself, nsDocShell::IsNavigationAllowed should return false, right? So why is the href set doing anything at all?

(In reply to Boris Zbarsky [:bz] from comment #14) > So wait. I'm a bit confused. > > During the beforeunload event itself, nsDocShell::IsNavigationAllowed should > return false, right? So why is the href set doing anything at all? because the href set calls into the xpcom endpoint LoadURI here: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1314 which never checks IsNavigationAllowed. I expect that checking that will Break Stuff (ie I guess that some APIs should be able to change the URI in a docshell even if IsNavigationAllowed returns false), but maybe I'm too much of a chicken? :-)

In case it matters, the top of the stack hitting PermitUnload ends up looking like: #0 0x00000001027349e4 in nsDocumentViewer::PermitUnloadInternal(bool, bool*, bool*) at /Users/gkruitbosch/dev/fx-team/layout/base/nsDocumentViewer.cpp:1084 #1 0x00000001027349b8 in nsDocumentViewer::PermitUnload(bool, bool*) at /Users/gkruitbosch/dev/fx-team/layout/base/nsDocumentViewer.cpp:1072 #2 0x0000000102aa7d82 in nsDocShell::InternalLoad(nsIURI*, nsIURI*, nsISupports*, unsigned int, char16_t const*, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString_internal const&, nsIDocShell*, nsIURI*, nsIDocShell**, nsIRequest**) at /Users/gkruitbosch/dev/fx-team/docshell/base/nsDocShell.cpp:9679 #3 0x0000000102aa5487 in nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) at /Users/gkruitbosch/dev/fx-team/docshell/base/nsDocShell.cpp:1590 #4 0x0000000101966aff in nsLocation::SetURI(nsIURI*, bool) at /Users/gkruitbosch/dev/fx-team/dom/base/nsLocation.cpp:274 #5 0x0000000101967a69 in nsLocation::SetHrefWithBase(nsAString_internal const&, nsIURI*, bool) at /Users/gkruitbosch/dev/fx-team/dom/base/nsLocation.cpp:560 #6 0x00000001019676ab in nsLocation::SetHrefWithContext(JSContext*, nsAString_internal const&, bool) [inlined] at /Users/gkruitbosch/dev/fx-team/dom/base/nsLocation.cpp:513 #7 0x0000000101967672 in nsLocation::SetHref(nsAString_internal const&) at /Users/gkruitbosch/dev/fx-team/dom/base/nsLocation.cpp:482 #8 0x0000000101c6d19b in nsLocation::SetHref(nsAString_internal const&, mozilla::ErrorResult&) [inlined] at /Users/gkruitbosch/dev/fx-team/dom/base/nsLocation.h:66 #9 0x0000000101c6d193 in mozilla::dom::LocationBinding::set_href(JSContext*, JS::Handle<JSObject*>, nsLocation*, JSJitSetterCallArgs) at /Users/gkruitbosch/dev/builds/opt-x86_64-apple-darwin13.3.0/dom/bindings/./LocationBinding.cpp:154 #10 0x0000000101c6aaa8 in mozilla::dom::LocationBinding::genericCrossOriginSetter(JSContext*, unsigned int, JS::Value*) at /Users/gkruitbosch/dev/builds/opt-x86_64-apple-darwin13.3.0/dom/bindings/./LocationBinding.cpp:973 #11 0x00000001035cff5c in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [inlined] at /Users/gkruitbosch/dev/fx-team/js/src/jscntxtinlines.h:231 #12 0x00000001035cff1b in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) at /Users/gkruitbosch/dev/fx-team/js/src/vm/Interpreter.cpp:474 #13 0x00000001035d04f2 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) at /Users/gkruitbosch/dev/fx-team/js/src/vm/Interpreter.cpp:537 #14 0x00000001035d09bf in js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) at /Users/gkruitbosch/dev/fx-team/js/src/vm/Interpreter.cpp:609

> because the href set calls into the xpcom endpoint LoadURI here: Whoa. I thought all the entrypoints called it! Certainly the discussion in bug 956524 was assuming that IsNavigationAllowed affects href sets, no? How about we just fix that? If we're too chicken to fix it in docshell, we should fix it in the Location code by asking the docshell.

And I guess tests for the various variants of bug 956524 would have been nice, but it's hard to automatic-test prompts. :(

(In reply to Boris Zbarsky [:bz] from comment #18) > And I guess tests for the various variants of bug 956524 would have been > nice, but it's hard to automatic-test prompts. :( Yes, although I found out when working on some add-on manager tests that you *can* actually do this by using the window watcher service and hooking into the alert prompt window... so let's add test coverage this time, at least...

There's a poignant comment at the top of that function about not checking mFiredUnloadEvent, so I jumped through some hoops. I can confirm this fixes the testcase attached to the bug. Boris, does this approach look right to you? I also considered factoring out the beforeunload check into its own function and calling both that and the isprintingorpp thing, but figured this was more useful than 'oh right, which functions am I meant to call to check if I can load something again' and having several of those. Asking for feedback only because I do think we should try to add some tests here, but I won't get to that before the weekend, I expect.

Marco, can you add this to the upcoming iteration, please? :-) (I don't know whether we've already requested new values for that Iteration field, but now would probably be a good time to get those, too...)

Comment on attachment 8481463 [details] [diff] [review] fix LoadURI to not let people load anything inside onbeforeunload, Yeah, if this works and ends up web-compatible and all this is great.

Try push of the above to see if this breaks anything obvious... remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7476dadbd37a

Added to the IT 35.1 priority backlog. New field values will be available for the Tuesday planning meeting. (In reply to :Gijs Kruitbosch from comment #21) > Marco, can you add this to the upcoming iteration, please? :-) > > (I don't know whether we've already requested new values for that Iteration > field, but now would probably be a good time to get those, too...)

(In reply to :Gijs Kruitbosch from comment #20) > Asking for > feedback only because I do think we should try to add some tests here, but I > won't get to that before the weekend, I expect. Yeah, so, this is harder than I thought. I'm not really sure why yet, although I have a suspicion the event queue spinning might be in the way. In any case, the thing I'm actually seeing is that: 1) I can successfully trigger the in-content dialog in my test by trying to load "about:blank", with the onbeforeunload handler trying to load "about:mozilla" 2) I listen for the "tabmodal-dialog-loaded" observer notification, which is sent by the dialog. 3a) I then try to click the "Stay on page" button from within the test, but this then leads to about:blank loading, despite clicking on 'stay on page'. Not sure why this happens. The same happens if I try to call some of the dialog's methods that the click would activate directly. 3b) if I comment out the attempt to click the button and click it myself, the test works as expected, although my mutation observer isn't detecting the dialog disappearing (this is a different problem... I imagine it's to do with all this being a XBL binding, but whatever) 3c) if I try to click the button from the test in a setTimeout, nothing happens and the dialog stays up (I expect this has to do with the event queue). I'm going to continue to poke at this.

Well, that was more effort than I anticipated. Here's what I hacked up. There's an ugly setTimeout(..., 0) in there but otherwise I'm reasonably happy with it. I've deferred on testing location.back/fwd/go(...) in here... I hope that's OK. :-\

Try push: remote: You can view the progress of your build at the following URL: remote: https://tbpl.mozilla.org/?tree=Try&rev=1033d6ada3f9 remote: Alternatively, view them on Treeherder (experimental): remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1033d6ada3f9

Comment on attachment 8483064 [details] [diff] [review] fix LoadURI to not let people load anything inside onbeforeunload, So the test will go orange if 3 seconds pass before "condition" is achieved. This seems like a lovely recipe for randomorange to me: we've seen instances of Firefox losing the timeslice for dozens of seconds at a time on our test infrastructure. :( Are we ok with that? It looks like waitForCondition is only used for waiting on the busy flags, right? Are those not set right by the relevant STATE_STOP notification? >+ setTimeout(runNextTest, 0); Can that be executeSoon? I'm not sure I follow how this test makes sure a load does _not_ happen.. Can you please explain that part? r=me for the rest

(In reply to Boris Zbarsky [:bz] from comment #28) > Comment on attachment 8483064 [details] [diff] [review] > fix LoadURI to not let people load anything inside onbeforeunload, > > So the test will go orange if 3 seconds pass before "condition" is achieved. > This seems like a lovely recipe for randomorange to me: we've seen instances > of Firefox losing the timeslice for dozens of seconds at a time on our test > infrastructure. :( > > Are we ok with that? No, but I don't have any better ideas. :-( > It looks like waitForCondition is only used for waiting on the busy flags, > right? Yes. > Are those not set right by the relevant STATE_STOP notification? AFAICT they are set by the OnStateChange handler in the docshell: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#6901 and whether that fires before or after "our" onstatechange/load handlers depends on implementation details of docshell and webprogress, which I don't really want to rely upon. Regardless, the simple current answer is "no, they are currently not yet set when the load event fires, or when onStateChange fires with STATE_STOP for a listener I add in the test" (which is also something I tried doing). As you can tell from the test, that's moderately annoying. > >+ setTimeout(runNextTest, 0); > > Can that be executeSoon? Probably! > I'm not sure I follow how this test makes sure a load does _not_ happen.. > Can you please explain that part? Assuming I understand the question correctly, if I back out the patch part here and keep the test, it fails because the busyFlags != 0 when the dialog opens, because at that point the location.reload/.href=foo/.replace have all been triggered already by the onbeforeunload event, which sync-triggers stuff loading. Thinking about this more... perhaps I can eliminate the evilness of the waitForCondition if I use a listener and check for the STATE_START notification that presumably triggers the changing of the busyFlags. I'll give this a go.

> AFAICT they are set by the OnStateChange handler in the docshell: Ah. So could we use our own onStateChange and then have it do an executeSoon to wait for other state change listeners (including docshell) to be notified?

(In reply to Boris Zbarsky [:bz] from comment #30) > > AFAICT they are set by the OnStateChange handler in the docshell: > > Ah. So could we use our own onStateChange and then have it do an > executeSoon to wait for other state change listeners (including docshell) to > be notified? Yes. However, I'll try to pursue the other suggestion (listening for STATE_START being fired) first, as I am hopeful that will avoid the executeSoon hack altogether...

This works, in my testing, and throws errors before recompiling docshell/base, and passes afterwards. :-)

remote: https://tbpl.mozilla.org/?tree=Try&rev=bd1a33869cd2 remote: Alternatively, view them on Treeherder (experimental): remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bd1a33869cd2

Comment on attachment 8485787 [details] [diff] [review] fix LoadURI to not let people load anything inside onbeforeunload, r=me

remote: https://hg.mozilla.org/integration/fx-team/rev/99d56dc4d825

I don't think we have a QA feature owner for this area specifically, but Cornel, your Tabs and Toolbars areas sound near enough, I hope you can take this one.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0 Mozilla/5.0 (X11; Linux i686; rv:35.0) Gecko/20100101 Firefox/35.0 Verified fixed on latest Nightly, build ID: 20140915030204 using the attached testcase.

Comment on attachment 8485787 [details] [diff] [review] fix LoadURI to not let people load anything inside onbeforeunload, Approval Request Comment [Feature/regressing bug #]: bug 616853 [User impact if declined]: in some cases, people get stuck on trap websites using onbeforeunload handlers [Describe test coverage new/current, TBPL]: now with automated test! [Risks and why]: medium - the current state is a clear regression, and the fix is simple and simple to back out, but there could be web compat impact, so I'm only asking for Aurora uplift and not beta. [String/UUID change made/needed]: nope

Comment on attachment 8485787 [details] [diff] [review] fix LoadURI to not let people load anything inside onbeforeunload, We will have the ability to hear about Web regressions while on Beta. I'm also setting ni on Karl and Hallvord to get their take on the Web compat risk. Aurora+

If I've understood this bug correctly, I think the *web* compat impact is going to be zero - it's basically a bug fix restoring functionality that's meant to work a certain way.

Then I'm not sure you understood this bug correctly. Without this patch, doing a location.href set in the beforeunload event handler would navigate to that location. With this patch, it becomes a no-op. That's a pretty significant behavior change; the only thing that makes us hopeful this is web-compatible is that other browsers seem to have that behavior already.

The behaviour change will only have web compat significance if people are doing it and expect it to work. Personally, I'd expect setting the location not to work in a beforeunload handler (much like alert() is forbidden). If browsers allow beforeunload and unload handlers to initiate navigation, they basically allow the page author to trap a user who wants to leave. At Opera this issue came up because the implementation on some platform at some point allowed something like that, we considered it a clear bug and fixed it. I thought this was even standardised, but can't find anything in the relevant specs (?!). So maybe I shouldn't state so confidently that it was meant to work this way ;) but I still think the compat risk is very low here.

(In reply to Hallvord R. M. Steen from comment #44) > If > browsers allow beforeunload and unload handlers to initiate navigation, they > basically allow the page author to trap a user who wants to leave. Well... there's also stuff like bug 510945. Having worked on a few of these bugs now (even after attempting to test other browsers' behaviour!) I am not sure we are 100% safe here (what happens if I assign a js URL to location.href, would you consider that less obviously broken?), hence proceeding with caution. Having it run a full beta cycle seems like the less risky version of fixing this. In any case, I'm glad you're optimistic! It would be sad (from a user control / security perspective) if we had to allow pages to do more here again, so I'm hopeful we can stick to this approach.

> if people are doing it and expect it to work. Right, that's the question. This is a web compat risk, not a known web compat issue. I agree the risk is low, fwiw. > I thought this was even standardised It is, but the navigation algorithm spec has had a number of web compat issues in the past, so we can't trust it too much so far, until all UAs actually completely align with it. :(

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:34.0) Gecko/20100101 Firefox/34.0 Mozilla/5.0 (X11; Linux i686; rv:34.0) Gecko/20100101 Firefox/34.0 Verified fixed on latest Aurora, build ID: 20140917004003 using the attached testcase.