Closed Bug 1004458 Opened 10 years ago Closed 10 years ago

quick refreshes of page with onbeforeunload can break page with setInterval and setTimeout

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Version:
29 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla32
Tracking Flags:
Tracking Status
firefox29 --- wontfix
firefox30 - verified
firefox31 --- verified
firefox32 --- verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: maartenroosen, Assigned: bzbarsky)

References

Details

(Keywords: regression, site-compat, testcase)

Attachments

(2 files)

onbeforeunload_refresh_test.html
418 bytes, text/html
Details
Maintain modal state on outer windows only instead of relying on forwarding from inner to outer
6.27 KB, patch
peterv
: review+
Gijs
: feedback+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140421221237

Steps to reproduce:

Make and open a HTML page using unbeforeunload like in this Gist: https://gist.github.com/mroosen/2c9a16c0d929ae283a0e

Now hold F5 / refresh key. While the "Are you sure?" page pops up, keep holding F5/ refresh key. Then click "Leave Page" a couple of times (10?) while holding F5.

(This happens sporadically at our customers, often by accident, refreshing after they have clicked "Leave Page" and the page is busy closing websockets and cleaning up.)

Tested on Windows 8.1


Actual results:

The defined intervals/timeouts don't start at all anymore after a couple of refreshes. And won't start up ON ANY SITE YOU VISIT after that. It looks like the whole tab where this happened won't work correctly after that. Other tabs are still OK.


Expected results:

The setTimeout and setInterval should keep working after (spam) refreshing a page that is using the onbeforeunload event.
I wonder if I broke this in bug 636374...


Maarten, was this issue not present in Firefox 28?
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
Flags: needinfo?(maartenroosen)
OS: Windows 8 → All
Product: Firefox → Core
Hardware: x86_64 → All
Summary: quick refreshes of page with unbeforeunload can break page with setInterval and setTimeout → quick refreshes of page with onbeforeunload can break page with setInterval and setTimeout
I've just installed 28 again to try it out, and yes; it also happens in 28!

It's such a simple way to break some serious functionality, I don't understand why this wasn't known before, I hope it can be fixed. 

Do you want any more information ?
Flags: needinfo?(maartenroosen)
(In reply to maartenroosen from comment #2)
> I've just installed 28 again to try it out, and yes; it also happens in 28!
> 
> It's such a simple way to break some serious functionality, I don't
> understand why this wasn't known before, I hope it can be fixed. 
> 
> Do you want any more information ?

Hrm. Well, if you would indulge me, it would be useful to check Firefox 26 and 27 as well - I just realized bug 636374 was uplifted to 27... so as to know exactly how long this has been broken, and where to look for the breaking change (which might still be bug 636374, or one of the other changes we made to how onbeforeunload is handled).

I do wonder what's broken though, because the setTimeout call still returns a number (for use with clearTimeout) - but the scheduled function never runs, it seems. I don't know why.
Flags: needinfo?(maartenroosen)
Keywords: testcase
26.0    OK
26.0b10 OK
27.0    (REALLY EASILY) BROKEN 
27.0b9  BROKEN

Probably something changed in 27.0 ?
Flags: needinfo?(maartenroosen)
Thanks! I will try to look at this as soon as I can.
Flags: needinfo?(gijskruitbosch+bugs)
We appreciate it!
Building locally gives:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0d11fce4f845&tochange=f00be67a992a

I suspect bug 933483. Doing some more builds and so on to be sure. Blake, does that sound plausible?
Blocks: 933483
Flags: needinfo?(mrbkap)
(In reply to :Gijs Kruitbosch from comment #8)
> Building locally gives:
> 
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=0d11fce4f845&tochange=f00be67a992a
> 
> I suspect bug 933483. Doing some more builds and so on to be sure. Blake,
> does that sound plausible?

Hrm, although updating to 832ef0e8771e locally, I can still reproduce... :-\

<more local building>

OK, I was wrong. It's b67b9a6a65fa, which is bug 938640, which I can't put in the dep tree here because I can't access it...
No longer blocks: 933483
Flags: needinfo?(peterv)
Flags: needinfo?(mrbkap)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bzbarsky)
Hmm.  That's pretty odd.  Are we ending up thinking we're running in a non-current inner all the time?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #10)
> Hmm.  That's pretty odd.  Are we ending up thinking we're running in a
> non-current inner all the time?

I just tried to look at this in a debug trunk build. You need to use the prompts.tab_modal.enabled pref to re-enable modal dialogs to quickly reproduce the issue there because onbeforeunload is now tab-modal.

In any case, with that, the only evidence I'm seeing of things gone awry is:

[6041] WARNING: Inner window does not have active document.: file /Users/gkruitbosch/dev/fx-team/dom/base/nsGlobalWindow.cpp, line 8540

Which is the bit added by bug 933483, not bug 938640...

I'm not sure what this warning ends up meaning, though. AFAICT we'd return from entermodalstate, so nothing else happens there (which I guess means the warnings are generated by showing the onbeforeunload dialog), and there doesn't seem to be anything in the debug spew related to the settimeout calls. :-\

I'm updating to the rev for 938640 itself and then probably apply xcode... we'll see if that gets me any closer to understanding this, I guess? If someone more knowledgable in these parts wants to take over, please do!
> nsGlobalWindow.cpp, line 8540

This is the "forward-to-outer" in LeaveModalState, right?  That totally explains what's going on.

> Which is the bit added by bug 933483, not bug 938640...

Bug 938640 changed the behavior of FORWARD_TO_OUTER such that non-current inners don't do the forwarding.

But this totally explains the issue, actually: if we end up trying to LeaveModalState on a non-current inner, we will now bail immediately.  In particular, we never decrement mModalStateDepth, so remain in modal state.

But RunTimeout will return without doing anything if IsInModalState().  So this in fact disables timeouts for that window...

Gijs, what does the C++ stack look like at the point when we hit that warning, if you can get it?

Peter, I think we should probably allow LeaveModalState on non-current inners to still forward to the outer...
Oh, and the reason I asked for the stack is I'm not sure who'd ever LeaveModalState on an inner, unless it's the nsAutoWindowStateHelper stuff in windowwatcher.
(In reply to Boris Zbarsky [:bz] from comment #12)
> > nsGlobalWindow.cpp, line 8540
> 
> This is the "forward-to-outer" in LeaveModalState, right?  That totally
> explains what's going on.

Yes, I got my enters and leaves mixed up while staring at this after midnight. Bad idea.

> > Which is the bit added by bug 933483, not bug 938640...
> 
> Bug 938640 changed the behavior of FORWARD_TO_OUTER such that non-current
> inners don't do the forwarding.
> 
> But this totally explains the issue, actually: if we end up trying to
> LeaveModalState on a non-current inner, we will now bail immediately.  In
> particular, we never decrement mModalStateDepth, so remain in modal state.

Ahhhhh. Yes.

> But RunTimeout will return without doing anything if IsInModalState().  So
> this in fact disables timeouts for that window...
> 
> Gijs, what does the C++ stack look like at the point when we hit that
> warning, if you can get it?

Something along these lines (I had to step through because debugging through the macro was a little messy and I wanted to make sure this was correct - I ended up setting a conditional breakpoint in LeaveModalState):

#2	0x000000010361becc in non-virtual thunk to nsGlobalWindow::LeaveModalState() at /Users/gkruitbosch/dev/fx-team/dom/base/nsGlobalWindow.cpp:8568
#3	0x0000000104ca3a42 in ~nsAutoWindowStateHelper at /Users/gkruitbosch/dev/fx-team/embedding/components/windowwatcher/src/nsAutoWindowStateHelper.cpp:38
#4	0x0000000104ca39d5 in ~nsAutoWindowStateHelper at /Users/gkruitbosch/dev/fx-team/embedding/components/windowwatcher/src/nsAutoWindowStateHelper.cpp:34
#5	0x0000000104ca88e4 in nsWindowWatcher::OpenWindowInternal(nsIDOMWindow*, char const*, char const*, char const*, bool, bool, bool, nsIArray*, nsIDOMWindow**) at /Users/gkruitbosch/dev/fx-team/embedding/components/windowwatcher/src/nsWindowWatcher.cpp:985
#6	0x0000000104ca58e9 in nsWindowWatcher::OpenWindow(nsIDOMWindow*, char const*, char const*, char const*, nsISupports*, nsIDOMWindow**) at /Users/gkruitbosch/dev/fx-team/embedding/components/windowwatcher/src/nsWindowWatcher.cpp:343
#7	0x00000001016ebc0b in NS_InvokeByIndex at /Users/gkruitbosch/dev/fx-team/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
#8	0x00000001034fe698 in CallMethodHelper::Invoke() at /Users/gkruitbosch/dev/fx-team/js/xpconnect/src/XPCWrappedNative.cpp:2397
#9	0x00000001034f2847 in CallMethodHelper::Call() at /Users/gkruitbosch/dev/fx-team/js/xpconnect/src/XPCWrappedNative.cpp:1738
#10	0x00000001034d170b in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) at /Users/gkruitbosch/dev/fx-team/js/xpconnect/src/XPCWrappedNative.cpp:1705
#11	0x00000001034d3cad in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) at /Users/gkruitbosch/dev/fx-team/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1273
#12	0x0000000106654f95 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) at /Users/gkruitbosch/dev/fx-team/js/src/jscntxtinlines.h:239
#13	0x00000001066290d1 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) at /Users/gkruitbosch/dev/fx-team/js/src/vm/Interpreter.cpp:475
#14	0x000000010661f116 in Interpret(JSContext*, js::RunState&) at /Users/gkruitbosch/dev/fx-team/js/src/vm/Interpreter.cpp:2620
#15	0x0000000106611ca9 in js::RunScript(JSContext*, js::RunState&) at /Users/gkruitbosch/dev/fx-team/js/src/vm/Interpreter.cpp:422
#16	0x00000001066291f1 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) at /Users/gkruitbosch/dev/fx-team/js/src/vm/Interpreter.cpp:494
#17	0x0000000106629a22 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:531
#18	0x00000001063a6b36 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) at /Users/gkruitbosch/dev/fx-team/js/src/jsapi.cpp:5194
#19	0x00000001034c73fe in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) at /Users/gkruitbosch/dev/fx-team/js/xpconnect/src/XPCWrappedJSClass.cpp:1272
#20	0x00000001034c113b in nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) at /Users/gkruitbosch/dev/fx-team/js/xpconnect/src/XPCWrappedJS.cpp:517
#21	0x00000001016ed712 in PrepareAndDispatch at /Users/gkruitbosch/dev/fx-team/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_darwin.cpp:122
#22	0x00000001016ec15b in SharedStub ()
#23	0x0000000104593fbc in nsDocumentViewer::PermitUnloadInternal(bool, bool*, bool*) at /Users/gkruitbosch/dev/fx-team/layout/base/nsDocumentViewer.cpp:1204
#24	0x000000010459332b in nsDocumentViewer::PermitUnload(bool, bool*) at /Users/gkruitbosch/dev/fx-team/layout/base/nsDocumentViewer.cpp:1062
#25	0x0000000104c630d9 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:9491
#26	0x0000000104c6bc32 in nsDocShell::LoadHistoryEntry(nsISHEntry*, unsigned int) at /Users/gkruitbosch/dev/fx-team/docshell/base/nsDocShell.cpp:11259
#27	0x0000000104c3948d in nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) at /Users/gkruitbosch/dev/fx-team/docshell/base/nsDocShell.cpp:1499
#28	0x0000000104c39b1e in non-virtual thunk to nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) at /Users/gkruitbosch/dev/fx-team/docshell/base/nsDocShell.cpp:1609
#29	0x0000000104c9b59f in nsSHistory::InitiateLoad(nsISHEntry*, nsIDocShell*, long) at /Users/gkruitbosch/dev/fx-team/docshell/shistory/src/nsSHistory.cpp:1789
#30	0x0000000104c9a9ed in nsSHistory::LoadEntry(int, long, unsigned int) at /Users/gkruitbosch/dev/fx-team/docshell/shistory/src/nsSHistory.cpp:1658
#31	0x0000000104c97d1f in nsSHistory::Reload(unsigned int) at /Users/gkruitbosch/dev/fx-team/docshell/shistory/src/nsSHistory.cpp:930
#32	0x0000000104c97d72 in non-virtual thunk to nsSHistory::Reload(unsigned int) at /Users/gkruitbosch/dev/builds/dbg-x86_64-apple-darwin13.1.0/docshell/shistory/src/Unified_cpp_shistory_src0.cpp:931
#33	0x00000001016ebc0b in NS_InvokeByIndex at /Users/gkruitbosch/dev/fx-team/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
#34	0x00000001034fe698 in CallMethodHelper::Invoke() at /Users/gkruitbosch/dev/fx-team/js/xpconnect/src/XPCWrappedNative.cpp:2397
#35	0x00000001034f2847 in CallMethodHelper::Call() at /Users/gkruitbosch/dev/fx-team/js/xpconnect/src/XPCWrappedNative.cpp:1738
#36	0x00000001034d170b in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) at /Users/gkruitbosch/dev/fx-team/js/xpconnect/src/XPCWrappedNative.cpp:1705
#37	0x00000001034d3cad in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) at /Users/gkruitbosch/dev/fx-team/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1273
#38	0x0000000106654f95 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) at /Users/gkruitbosch/dev/fx-team/js/src/jscntxtinlines.h:239
#39	0x00000001066290d1 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) at /Users/gkruitbosch/dev/fx-team/js/src/vm/Interpreter.cpp:475
#40	0x000000010661f116 in Interpret(JSContext*, js::RunState&) at /Users/gkruitbosch/dev/fx-team/js/src/vm/Interpreter.cpp:2620
#41	0x0000000106611ca9 in js::RunScript(JSContext*, js::RunState&) at /Users/gkruitbosch/dev/fx-team/js/src/vm/Interpreter.cpp:422
#42	0x00000001066291f1 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) at /Users/gkruitbosch/dev/fx-team/js/src/vm/Interpreter.cpp:494
#43	0x0000000106629a22 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:531
#44	0x00000001063a6c54 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) at /Users/gkruitbosch/dev/fx-team/js/src/jsapi.cpp:5206
#45	0x0000000102b52557 in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) at /Users/gkruitbosch/dev/builds/dbg-x86_64-apple-darwin13.1.0/dom/bindings/./EventHandlerBinding.cpp:35
#46	0x00000001037d3bc8 in JS::Value mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling) at /Users/gkruitbosch/dev/builds/dbg-x86_64-apple-darwin13.1.0/dom/events/../../dist/include/mozilla/dom/EventHandlerBinding.h:62
#47	0x00000001037c5d8d in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) at /Users/gkruitbosch/dev/fx-team/dom/events/JSEventHandler.cpp:205
#48	0x00000001037a30f3 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) at /Users/gkruitbosch/dev/fx-team/dom/events/EventListenerManager.cpp:950
#49	0x00000001037a3416 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) at /Users/gkruitbosch/dev/fx-team/dom/events/EventListenerManager.cpp:1011
#50	0x00000001037bc399 in mozilla::EventListenerManager::HandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) at /Users/gkruitbosch/dev/builds/dbg-x86_64-apple-darwin13.1.0/dom/events/../../dist/include/mozilla/EventListenerManager.h:327
#51	0x00000001037a9d7f in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) at /Users/gkruitbosch/dev/fx-team/dom/events/EventDispatcher.cpp:194
#52	0x000000010379d2db in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) at /Users/gkruitbosch/dev/fx-team/dom/events/EventDispatcher.cpp:287
#53	0x000000010379eb27 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsCOMArray<mozilla::dom::EventTarget>*) at /Users/gkruitbosch/dev/fx-team/dom/events/EventDispatcher.cpp:597
#54	0x0000000103783c4a in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) at /Users/gkruitbosch/dev/fx-team/dom/events/EventDispatcher.cpp:661
#55	0x0000000103d3c432 in nsINode::DispatchEvent(nsIDOMEvent*, bool*) at /Users/gkruitbosch/dev/fx-team/content/base/src/nsINode.cpp:1236
#56	0x0000000103c2ccfd in nsContentUtils::DispatchXULCommand(nsIContent*, bool, nsIDOMEvent*, nsIPresShell*, bool, bool, bool, bool) at /Users/gkruitbosch/dev/fx-team/content/base/src/nsContentUtils.cpp:5590
#57	0x00000001042603a1 in nsXULElement::PreHandleEvent(mozilla::EventChainPreVisitor&) at /Users/gkruitbosch/dev/fx-team/content/xul/content/src/nsXULElement.cpp:1205
#58	0x000000010379cf43 in mozilla::EventTargetChainItem::PreHandleEvent(mozilla::EventChainPreVisitor&) at /Users/gkruitbosch/dev/fx-team/dom/events/EventDispatcher.cpp:230
#59	0x000000010379e57d in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsCOMArray<mozilla::dom::EventTarget>*) at /Users/gkruitbosch/dev/fx-team/dom/events/EventDispatcher.cpp:525
#60	0x0000000103783c4a in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) at /Users/gkruitbosch/dev/fx-team/dom/events/EventDispatcher.cpp:661
#61	0x0000000103d3c432 in nsINode::DispatchEvent(nsIDOMEvent*, bool*) at /Users/gkruitbosch/dev/fx-team/content/base/src/nsINode.cpp:1236
#62	0x0000000103c2ccfd in nsContentUtils::DispatchXULCommand(nsIContent*, bool, nsIDOMEvent*, nsIPresShell*, bool, bool, bool, bool) at /Users/gkruitbosch/dev/fx-team/content/base/src/nsContentUtils.cpp:5590
#63	0x0000000103ad981c in nsXBLPrototypeHandler::DispatchXULKeyCommand(nsIDOMEvent*) at /Users/gkruitbosch/dev/fx-team/dom/xbl/nsXBLPrototypeHandler.cpp:558
#64	0x0000000103ac2a36 in nsXBLPrototypeHandler::ExecuteHandler(mozilla::dom::EventTarget*, nsIDOMEvent*) at /Users/gkruitbosch/dev/fx-team/dom/xbl/nsXBLPrototypeHandler.cpp:223
#65	0x0000000103afd75f in nsXBLWindowKeyHandler::WalkHandlersAndExecute(nsIDOMKeyEvent*, nsIAtom*, nsXBLPrototypeHandler*, unsigned int, bool, bool) at /Users/gkruitbosch/dev/fx-team/dom/xbl/nsXBLWindowKeyHandler.cpp:531
#66	0x0000000103afcadc in nsXBLWindowKeyHandler::WalkHandlersInternal(nsIDOMKeyEvent*, nsIAtom*, nsXBLPrototypeHandler*, bool) at /Users/gkruitbosch/dev/fx-team/dom/xbl/nsXBLWindowKeyHandler.cpp:444
#67	0x0000000103afc944 in nsXBLWindowKeyHandler::WalkHandlers(nsIDOMKeyEvent*, nsIAtom*) at /Users/gkruitbosch/dev/fx-team/dom/xbl/nsXBLWindowKeyHandler.cpp:296
#68	0x0000000103afcd8d in nsXBLWindowKeyHandler::HandleEvent(nsIDOMEvent*) at /Users/gkruitbosch/dev/fx-team/dom/xbl/nsXBLWindowKeyHandler.cpp:319
#69	0x00000001037a30f3 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) at /Users/gkruitbosch/dev/fx-team/dom/events/EventListenerManager.cpp:950
#70	0x00000001037a3416 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) at /Users/gkruitbosch/dev/fx-team/dom/events/EventListenerManager.cpp:1011
#71	0x00000001037bc399 in mozilla::EventListenerManager::HandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) at /Users/gkruitbosch/dev/builds/dbg-x86_64-apple-darwin13.1.0/dom/events/../../dist/include/mozilla/EventListenerManager.h:327
#72	0x00000001037a9d7f in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) at /Users/gkruitbosch/dev/fx-team/dom/events/EventDispatcher.cpp:194
#73	0x000000010379d427 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) at /Users/gkruitbosch/dev/fx-team/dom/events/EventDispatcher.cpp:308
#74	0x000000010379d5c4 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) at /Users/gkruitbosch/dev/fx-team/dom/events/EventDispatcher.cpp:336
#75	0x000000010379eb27 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsCOMArray<mozilla::dom::EventTarget>*) at /Users/gkruitbosch/dev/fx-team/dom/events/EventDispatcher.cpp:597
#76	0x00000001045c44dc in PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*) at /Users/gkruitbosch/dev/fx-team/layout/base/nsPresShell.cpp:7404
#77	0x00000001045c29be in PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) at /Users/gkruitbosch/dev/fx-team/layout/base/nsPresShell.cpp:7021
#78	0x00000001045c0c69 in PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) at /Users/gkruitbosch/dev/fx-team/layout/base/nsPresShell.cpp:6591
#79	0x0000000103ba79c1 in nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) at /Users/gkruitbosch/dev/fx-team/view/src/nsViewManager.cpp:774
#80	0x0000000103ba4aff in nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) at /Users/gkruitbosch/dev/fx-team/view/src/nsView.cpp:1097
#81	0x00000001032d2958 in nsChildView::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) at /Users/gkruitbosch/dev/fx-team/widget/cocoa/nsChildView.mm:1675
#82	0x00000001032d2a06 in nsChildView::DispatchWindowEvent(mozilla::WidgetGUIEvent&) at /Users/gkruitbosch/dev/fx-team/widget/cocoa/nsChildView.mm:1683
#83	0x00000001033029c9 in mozilla::widget::TextInputHandlerBase::DispatchEvent(mozilla::WidgetGUIEvent&) at /Users/gkruitbosch/dev/fx-team/widget/cocoa/TextInputHandler.mm:4245
#84	0x00000001033020f4 in mozilla::widget::TextInputHandler::HandleKeyDownEvent(NSEvent*) at /Users/gkruitbosch/dev/fx-team/widget/cocoa/TextInputHandler.mm:1594
#85	0x00000001032e6ce5 in -[ChildView keyDown:] at /Users/gkruitbosch/dev/fx-team/widget/cocoa/nsChildView.mm:5471
#86	0x00007fff8bcf465b in -[NSWindow sendEvent:] ()
#87	0x000000010332d2a1 in -[ToolbarWindow sendEvent:] at /Users/gkruitbosch/dev/fx-team/widget/cocoa/nsCocoaWindow.mm:3295
#88	0x00007fff8bc95c42 in -[NSApplication sendEvent:] ()
#89	0x0000000103319bd2 in -[GeckoNSApplication sendEvent:] at /Users/gkruitbosch/dev/fx-team/widget/cocoa/nsAppShell.mm:57
#90	0x00007fff8bae5b89 in -[NSApplication run] ()
#91	0x000000010331b502 in nsAppShell::Run() at /Users/gkruitbosch/dev/fx-team/widget/cocoa/nsAppShell.mm:546
#92	0x00000001050749dc in nsAppStartup::Run() at /Users/gkruitbosch/dev/fx-team/toolkit/components/startup/nsAppStartup.cpp:278
#93	0x0000000104f2d766 in XREMain::XRE_mainRun() at /Users/gkruitbosch/dev/fx-team/toolkit/xre/nsAppRunner.cpp:4023
#94	0x0000000104f2dfce in XREMain::XRE_main(int, char**, nsXREAppData const*) at /Users/gkruitbosch/dev/fx-team/toolkit/xre/nsAppRunner.cpp:4092
#95	0x0000000104f2e46d in XRE_main at /Users/gkruitbosch/dev/fx-team/toolkit/xre/nsAppRunner.cpp:4304
#96	0x0000000100001f8f in do_main(int, char**, nsIFile*) at /Users/gkruitbosch/dev/fx-team/browser/app/nsBrowserApp.cpp:282
#97	0x00000001000014b3 in main at /Users/gkruitbosch/dev/fx-team/browser/app/nsBrowserApp.cpp:643
Egh, missed the top two:

#0	0x00000001015f126e in NS_DebugBreak at /Users/gkruitbosch/dev/fx-team/xpcom/base/nsDebugImpl.cpp:355
#1	0x000000010361bc73 in nsGlobalWindow::LeaveModalState() at /Users/gkruitbosch/dev/fx-team/dom/base/nsGlobalWindow.cpp:8540

So anyway, yes, this is coming from nsAutoWindowStateHelper. :-)
Hrm.  That stack should be handing an outer window to the nsAutoWindowStateHelper, I would have thought!
(In reply to Boris Zbarsky [:bz] from comment #16)
> Hrm.  That stack should be handing an outer window to the
> nsAutoWindowStateHelper, I would have thought!

Hrm. So if I examine the stack at the nsAutoWindowStateHelper's destructor, I see:

mWindow	nsGlobalWindow *	0x115dcbc00	0x0000000115dcbc00
with:
nsPIDOMWindow	nsPIDOMWindow
with:
mIsInnerWindow	bool	true	true
and:
mInnerWindow	nsPIDOMWindow *	NULL	0x0000000000000000

Ditto for the aParent that is passed around in OpenWindow(). Which makes it seem that the call for the onbeforeunload dialog creates the dialog with the inner window as parent.

Sadly, all my attempts to call DumpJSStack() produce no output at all. So I'm not sure where this window is coming from, and why it's an inner rather than an outer window. (then again, I'm also not sure why you particularly expect an outer rather than an inner window!)
I expected aParent to be the outer because that's what the calling script has, but of course XPConnect unwrapping innerizes.  Taking this.  Patch coming up.  I still haven't managed to reproduce this, so would appreciate testing on the patch...
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8421863 [details] [diff] [review]
Maintain modal state on outer windows only instead of relying on forwarding from inner to outer

Review of attachment 8421863 [details] [diff] [review]:
-----------------------------------------------------------------

::: embedding/components/windowwatcher/src/nsAutoWindowStateHelper.cpp
@@ +23,4 @@
>    : mWindow(aWindow),
>      mDefaultEnabled(DispatchEventToChrome("DOMWillOpenModalDialog"))
>  {
> +  if (mWindow) {

Assert outer here too, please.
Flags: needinfo?(peterv) → needinfo?(gijskruitbosch+bugs)
Comment on attachment 8421863 [details] [diff] [review]
Maintain modal state on outer windows only instead of relying on forwarding from inner to outer

This patch seems to fix the issue for me.
Attachment #8421863 - Flags: feedback+
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [need review]
Attachment #8421863 - Flags: review?(peterv) → review+
Er, yes, because parentWindow can be null.  I'll add a null-check.
Ugh, missed that.
This push caused a large number of B2G debug mochitest failures that contributed to a large bustage pileup on inbound and led to having to revert to a last-good cset, inconveniencing other innocent developers as well. These same failures occurred with yesterday's landing. Please give this a full Try run before attempting to re-land.

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2a6f7785a2

https://tbpl.mozilla.org/php/getParsedLog.php?id=39821101&tree=Mozilla-Inbound
Comment on attachment 8421863 [details] [diff] [review]
Maintain modal state on outer windows only instead of relying on forwarding from inner to outer

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 938640
User impact if declined: Possible to get the browser into a state where the web
   doesn't really work very well.  ;)
Testing completed (on m-c, etc.): Tests are green.
Risk to taking this patch (and alternatives if risky): I think risk is low to
   moderate.  The only viable alternative I see is letting it ride the trains.
String or IDL/UUID changes made by this patch:  None.
Attachment #8421863 - Flags: approval-mozilla-beta?
Attachment #8421863 - Flags: approval-mozilla-aurora?
Flags: in-testsuite?
Whiteboard: [need review]
https://hg.mozilla.org/mozilla-central/rev/795c41afe2a6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Attachment #8421863 - Flags: approval-mozilla-beta?
Attachment #8421863 - Flags: approval-mozilla-beta+
Attachment #8421863 - Flags: approval-mozilla-aurora?
Attachment #8421863 - Flags: approval-mozilla-aurora+
Reproduced in Nightly 2014-05-17 with prompts.tab_modal.enabled=FALSE.
Verified fixed 32.0a1 (2014-05-19), Win 7 x64.
Status: RESOLVED → VERIFIED
status-firefox32: fixed → verified
Keywords: verifyme
> The b2g bits were bug 1011810.

I meant they were caused by bug 1008236...  Sorry for the wrong bug number.
We're taking the uplift but this doesn't look like a critical issue enough to track.
tracking-firefox30: ? → -
Verified as fixed on Firefox 30 - build ID: 20140605174243 and Firefox 31 beta 6 - build ID: 20140630185627.
Tested on Mac OS X 10.8.5, Windows 7 64bit and Ubuntu 13.04.
status-firefox30: fixed → verified
status-firefox31: fixed → verified
Keywords: verifyme
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: