579535 - Remove AddScriptBlockerAndPreventAddingRunners() API and make nsDocument's onload blocking safer

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

When nsContentUtils::sScriptBlockerCountWhereRunnersPrevented is > 0, script runners are rejected by AddScriptRunner:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#4660

This can only be set by nsContentUtils::AddScriptBlockerAndPreventAddingRunners(), whose sole consumer (scrolling) was eliminated when retained layers landed yesterday.

However, up until then, we'd reject script runners during a scroll. This breaks the onload blocking machinery of nsDocument.cpp. If we determine that we can't synchronously block onload, we set a deferred (async) block and add a script runner to switch the deferred block to an actual block:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#6852

If this script runner is rejected, we never switch the deferred block(s) over to real block(s). This is true even if we call BlockOnload again, since we only spawn the script runner for the _first_ deferred block.

When somebody then tries to unblock onload, we notice that there are still pending async notifications, and thus post ourselves back to the event queue to try again:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#6935

This causes us to thrash endlessly, making firefox use 100% of its available CPU (50% on dual-core). This was the cause of bug 576615.

This problem is dormant for now thanks to retained layers. Here's what I think we should to make sure it doesn't happen again:

1) Remove the AddScriptBlockerAndPreventAddingRunners() API, and add ensure that AddScriptRunner() always succeeds.

2) Assert success of adding deferred block script runners from nsDocument.

3) Make blocking/unblocking onload a no-op if onload has already been fired.

Comment 1

[Bobby Holley (:bholley)]

Attachment: patch v1

Added a patch. Pushed to try as f74f9a5dc41a, and flagging bz for review.

Comment 2

[Bobby Holley (:bholley)]

This just went orange on try.

The two tests that failed were
layout/reftests/bugs/388980-1.html
and
docshell/test/navigation/test_bug386782.html

both of them have to do with design mode.

The only functional change in this patch is adding this at the beginning of BlockOnload and UnblockOnload:

+
+  // No-op if the document is already loaded
+  nsPIDOMWindow *window = GetWindow();
+  if (!window || !window->IsLoading())
+    return;

I'm asking ehsan if he knows why designmode documents would either not have a window or have something weird about their loading.

Comment 3

[(no longer active)]

As I discussed this with Bobby, I think this patch is wrong, because it changes the behavior of the document in the cases where the load event is not called and the document is already loaded when BlockOnload is called (when restoring stuff from the bfcache, possibly among others).

This is the case with this test:

http://mxr.mozilla.org/mozilla-central/source/docshell/test/navigation/test_bug386782.html?force=1

I saw something in the docshell code when debugging bug 289384 with bz over IRC, but I can't find it right now for the life of me.  Bobby, can you set a breakpoint in nsDocShell::CanSavePresentation to see what causes it to return false?  Especially look for the stuff having to do with the document's load group...

Comment 4

[Boris Zbarsky [:bzbarsky]]

window->IsLoading() becomes false before onload finishes firing, for one thing.  Other than that, I don't see obvious issues that could arise as a result of that hunk, offhand.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Removing AddScriptBlockerAndPreventAddingRunners is definitely a good call now that we don't need it anymore.

Comment 6

[Bobby Holley (:bholley)]

I got partway through debugging this. Unfortunately, given beltzner's recent email, it doesn't sound like I should be working on this until september. So in reality, given my school schedule, that means there's a good chance won't get back to it until a year from now. :(

The reftest failure is the result of a new assertion, where we assert because we call UnblockOnload once too many times. I've investigated this, and it appears to be that we bail early on the following BlockOnload (making it a no-op), but don't do so on the corresponding UnblockOnload:

#0  nsDocument::BlockOnload (this=0x101486200) at /files/mozilla/src/content/base/src/nsDocument.cpp:6847
#1  0x000000011384c58b in nsBindingManager::PostProcessAttachedQueueEvent (this=0x123962e00) at /files/mozilla/src/content/xbl/src/nsBindingManager.cpp:969
#2  0x000000011384c6da in nsBindingManager::AddToAttachedQueue (this=0x123962e00, aBinding=0x12399b930) at /files/mozilla/src/content/xbl/src/nsBindingManager.cpp:955
#3  0x0000000113242b20 in nsFrameConstructorState::~nsFrameConstructorState (this=0x7fff5fbf8ca0) at /files/mozilla/src/layout/base/nsCSSFrameConstructor.cpp:995
#4  0x000000011324aaa2 in nsCSSFrameConstructor::SetUpDocElementContainingBlock (this=0x123886be0, aDocElement=0x123962070) at /files/mozilla/src/layout/base/nsCSSFrameConstructor.cpp:2836
#5  0x000000011324c94d in nsCSSFrameConstructor::ConstructDocElementFrame (this=0x123886be0, aDocElement=0x123962070, aFrameState=0x1238954e0, aNewFrame=0x7fff5fbf9290) at /files/mozilla/src/layout/base/nsCSSFrameConstructor.cpp:2331
#6  0x000000011324d97c in nsCSSFrameConstructor::ContentRangeInserted (this=0x123886be0, aContainer=0x0, aStartChild=0x123962070, aEndChild=0x0, aFrameState=0x1238954e0, aAllowLazyConstruction=0) at /files/mozilla/src/layout/base/nsCSSFrameConstructor.cpp:6894
#7  0x000000011324eb35 in nsCSSFrameConstructor::ContentInserted (this=0x123886be0, aContainer=0x0, aChild=0x123962070, aFrameState=0x1238954e0, aAllowLazyConstruction=0) at /files/mozilla/src/layout/base/nsCSSFrameConstructor.cpp:6793
#8  0x000000011324fb82 in nsCSSFrameConstructor::RecreateFramesForContent (this=0x123886be0, aContent=0x123962070, aAsyncInsert=0) at /files/mozilla/src/layout/base/nsCSSFrameConstructor.cpp:9084
#9  0x0000000113250602 in nsCSSFrameConstructor::ProcessRestyledFrames (this=0x123886be0, aChangeList=@0x7fff5fbf9500) at /files/mozilla/src/layout/base/nsCSSFrameConstructor.cpp:7949
#10 0x0000000113250dd8 in nsCSSFrameConstructor::RestyleElement (this=0x123886be0, aElement=0x123962070, aPrimaryFrame=0x1019845d0, aMinHint=0, aRestyleTracker=@0x123886c90, aRestyleDescendants=1) at /files/mozilla/src/layout/base/nsCSSFrameConstructor.cpp:8039
#11 0x00000001132370aa in mozilla::css::RestyleTracker::ProcessOneRestyle (this=0x123886c90, aElement=0x123962070, aRestyleHint=eRestyle_Subtree, aChangeHint=0) at /files/mozilla/src/layout/base/RestyleTracker.cpp:156
#12 0x0000000113235c2e in mozilla::css::RestyleTracker::ProcessRestyles (this=0x123886c90) at /files/mozilla/src/layout/base/RestyleTracker.cpp:238
#13 0x0000000113250b69 in nsCSSFrameConstructor::ProcessPendingRestyles (this=0x123886be0) at /files/mozilla/src/layout/base/nsCSSFrameConstructor.cpp:11603
#14 0x00000001132cedd2 in PresShell::FlushPendingNotifications (this=0x123844320, aType=Flush_Style) at /files/mozilla/src/layout/base/nsPresShell.cpp:4786
#15 0x000000011359124d in nsDocument::FlushPendingNotifications (this=0x101486200, aType=Flush_Style) at /files/mozilla/src/content/base/src/nsDocument.cpp:6120
#16 0x0000000113778463 in nsHTMLDocument::EditingStateChanged (this=0x101486200) at /files/mozilla/src/content/html/document/src/nsHTMLDocument.cpp:3333
#17 0x0000000113778c3c in nsHTMLDocument::SetDesignMode (this=0x101486200, aDesignMode=@0x7fff5fbfa840) at /files/mozilla/src/content/html/document/src/nsHTMLDocument.cpp:3423
#18 0x0000000113e4576d in nsIDOMNSHTMLDocument_SetDesignMode (cx=0x11f188350, obj=0x12378dec0, id=4889833092, vp=0x7fff5fbfb0c0) at dom_quickstubs.cpp:19432
#19 0x00000001000c592d in js::callJSPropertyOpSetter (cx=0x11f188350, op=0x113e45643 <nsIDOMNSHTMLDocument_SetDesignMode(JSContext*, JSObject*, long, long*)>, obj=0x12378dec0, idval=4889833092, vp=0x7fff5fbfb0c0) at jscntxtinlines.h:369
#20 0x0000000100156642 in JSScopeProperty::set (this=0x101bf8e28, cx=0x11f188350, obj=0x12378dec0, vp=0x7fff5fbfb0c0) at jsscopeinlines.h:310
#21 0x000000010014a1db in js_SetPropertyHelper (cx=0x11f188350, obj=0x12378dec0, id=4889833092, defineHow=1, vp=0x7fff5fbfb0c0) at /files/mozilla/src/js/src/jsobj.cpp:5122
#22 0x000000010011b360 in js_Interpret (cx=0x11f188350) at jsops.cpp:1824
#23 0x000000010012fca7 in Invoke<int (*)(JSContext*, JSObject*, unsigned int, long*, long*)> (cx=0x11f188350, fun=0x1237917e0, script=0x12398f130, native=0, args=@0x7fff5fbfb550, flags=0) at jsinterp.cpp:602
#24 0x0000000100132648 in js_Invoke (cx=0x11f188350, args=@0x7fff5fbfb550, flags=0) at jsinterp.cpp:693
#25 0x0000000100132c65 in js_InternalInvoke (cx=0x11f188350, thisv=4890108800, fval=4890108864, flags=0, argc=1, argv=0x101427820, rval=0x7fff5fbfb718) at jsinterp.cpp:739
#26 0x000000010008b0f7 in JS_CallFunctionValue (cx=0x11f188350, obj=0x123792380, fval=4890108864, argc=1, argv=0x101427820, rval=0x7fff5fbfb718) at /files/mozilla/src/js/src/jsapi.cpp:4850
#27 0x0000000113892d77 in nsJSContext::CallEventHandler (this=0x11f185cc0, aTarget=0x12386a6d0, aScope=0x12374f840, aHandler=0x1237923c0, aargv=0x123990160, arv=0x7fff5fbfb950) at /files/mozilla/src/dom/base/nsJSEnvironment.cpp:2204
#28 0x000000011391a197 in nsJSEventListener::HandleEvent (this=0x123869c20, aEvent=0x12398ef50) at /files/mozilla/src/dom/src/events/nsJSEventListener.cpp:228
#29 0x000000011368d6d8 in nsEventListenerManager::HandleEventSubType (this=0x100a3d750, aListenerStruct=0x100a3d798, aListener=0x123869c20, aDOMEvent=0x12398ef50, aCurrentTarget=0x12386a6d0, aPhaseFlags=6, aPusher=0x7fff5fbfbff0) at /files/mozilla/src/content/events/src/nsEventListenerManager.cpp:1094
#30 0x000000011368db50 in nsEventListenerManager::HandleEventInternal (this=0x100a3d750, aPresContext=0x0, aEvent=0x7fff5fbfc0e0, aDOMEvent=0x7fff5fbfbfd0, aCurrentTarget=0x12386a6d0, aFlags=6, aEventStatus=0x7fff5fbfbfd8, aPusher=0x7fff5fbfbff0) at /files/mozilla/src/content/events/src/nsEventListenerManager.cpp:1190
#31 0x00000001136bc97b in nsEventListenerManager::HandleEvent (this=0x100a3d750, aPresContext=0x0, aEvent=0x7fff5fbfc0e0, aDOMEvent=0x7fff5fbfbfd0, aCurrentTarget=0x12386a6d0, aFlags=6, aEventStatus=0x7fff5fbfbfd8, aPusher=0x7fff5fbfbff0) at nsEventListenerManager.h:146
#32 0x00000001136bcb26 in nsEventTargetChainItem::HandleEvent (this=0x1019bd9c0, aVisitor=@0x7fff5fbfbfc0, aFlags=6, aMayHaveNewListenerManagers=0, aPusher=0x7fff5fbfbff0) at /files/mozilla/src/content/events/src/nsEventDispatcher.cpp:212
#33 0x00000001136baf5b in nsEventTargetChainItem::HandleEventTargetChain (this=0x1019bd988, aVisitor=@0x7fff5fbfbfc0, aFlags=6, aCallback=0x0, aMayHaveNewListenerManagers=0, aPusher=0x7fff5fbfbff0) at /files/mozilla/src/content/events/src/nsEventDispatcher.cpp:341
#34 0x00000001136bbbf2 in nsEventDispatcher::Dispatch (aTarget=0x12386a6d0, aPresContext=0x0, aEvent=0x7fff5fbfc0e0, aDOMEvent=0x0, aEventStatus=0x7fff5fbfc158, aCallback=0x0, aTargets=0x0) at /files/mozilla/src/content/events/src/nsEventDispatcher.cpp:628
#35 0x00000001138c21bd in nsGlobalWindow::PostHandleEvent (this=0x123963840, aVisitor=@0x7fff5fbfc340) at /files/mozilla/src/dom/base/nsGlobalWindow.cpp:2386
#36 0x00000001136bad3d in nsEventTargetChainItem::PostHandleEvent (this=0x1019bd8e0, aVisitor=@0x7fff5fbfc340) at /files/mozilla/src/content/events/src/nsEventDispatcher.cpp:286
#37 0x00000001136baf74 in nsEventTargetChainItem::HandleEventTargetChain (this=0x1019bd870, aVisitor=@0x7fff5fbfc340, aFlags=518, aCallback=0x0, aMayHaveNewListenerManagers=0, aPusher=0x7fff5fbfc370) at /files/mozilla/src/content/events/src/nsEventDispatcher.cpp:344
#38 0x00000001136bb155 in nsEventTargetChainItem::HandleEventTargetChain (this=0x1019bd870, aVisitor=@0x7fff5fbfc340, aFlags=6, aCallback=0x0, aMayHaveNewListenerManagers=0, aPusher=0x7fff5fbfc370) at /files/mozilla/src/content/events/src/nsEventDispatcher.cpp:395
#39 0x00000001136bbbf2 in nsEventDispatcher::Dispatch (aTarget=0x123958500, aPresContext=0x123886460, aEvent=0x7fff5fbfc460, aDOMEvent=0x0, aEventStatus=0x7fff5fbfc4c4, aCallback=0x0, aTargets=0x0) at /files/mozilla/src/content/events/src/nsEventDispatcher.cpp:628
#40 0x00000001132864bf in DocumentViewerImpl::LoadComplete (this=0x123962f90, aStatus=0) at /files/mozilla/src/layout/base/nsDocumentViewer.cpp:1036
#41 0x0000000115b4b271 in nsDocShell::EndPageLoad (this=0x12386ac70, aProgress=0x12386ac98, aChannel=0x12395d6a0, aStatus=0) at /files/mozilla/src/docshell/base/nsDocShell.cpp:5794
#42 0x0000000115b45676 in nsDocShell::OnStateChange (this=0x12386ac70, aProgress=0x12386ac98, aRequest=0x12395d6a0, aStateFlags=131088, aStatus=0) at /files/mozilla/src/docshell/base/nsDocShell.cpp:5654
#43 0x0000000115b88574 in nsDocLoader::FireOnStateChange (this=0x12386ac70, aProgress=0x12386ac98, aRequest=0x12395d6a0, aStateFlags=131088, aStatus=0) at /files/mozilla/src/uriloader/base/nsDocLoader.cpp:1321
#44 0x0000000115b88bde in nsDocLoader::doStopDocumentLoad (this=0x12386ac70, request=0x12395d6a0, aStatus=0) at /files/mozilla/src/uriloader/base/nsDocLoader.cpp:929
#45 0x0000000115b88eec in nsDocLoader::DocLoaderIsEmpty (this=0x12386ac70, aFlushLayout=1) at /files/mozilla/src/uriloader/base/nsDocLoader.cpp:805
#46 0x0000000115b8a16e in nsDocLoader::OnStopRequest (this=0x12386ac70, aRequest=0x123962280, aCtxt=0x0, aStatus=0) at /files/mozilla/src/uriloader/base/nsDocLoader.cpp:700
#47 0x0000000112163383 in nsLoadGroup::RemoveRequest (this=0x12386af90, request=0x123962280, ctxt=0x0, aStatus=0) at /files/mozilla/src/netwerk/base/src/nsLoadGroup.cpp:680
#48 0x00000001135ad05b in nsDocument::DoUnblockOnload (this=0x101486200) at /files/mozilla/src/content/base/src/nsDocument.cpp:6963
#49 0x00000001135ad1bd in nsDocument::UnblockOnload (this=0x101486200, aFireSync=1) at /files/mozilla/src/content/base/src/nsDocument.cpp:6905
#50 0x000000011384c658 in nsBindingManager::DoProcessAttachedQueue (this=0x123962e00) at /files/mozilla/src/content/xbl/src/nsBindingManager.cpp:995
#51 0x000000011384fd2f in nsRunnableMethodImpl<void (nsBindingManager::*)(), true>::Run (this=0x123895720) at nsThreadUtils.h:347
#52 0x00000001005787ac in nsThread::ProcessNextEvent (this=0x100a04820, mayWait=0, result=0x7fff5fbfcfe4) at /files/mozilla/src/xpcom/threads/nsThread.cpp:547
#53 0x00000001004f06f7 in NS_ProcessPendingEvents_P (thread=0x100a04820, timeout=20) at nsThreadUtils.cpp:200
#54 0x0000000113023278 in nsBaseAppShell::NativeEventCallback (this=0x100936bd0) at /files/mozilla/src/widget/src/xpwidgets/nsBaseAppShell.cpp:126
#55 0x0000000112fd0bae in nsAppShell::ProcessGeckoEvents (aInfo=0x100936bd0) at /files/mozilla/src/widget/src/cocoa/nsAppShell.mm:394
#56 0x00007fff84c7b281 in __CFRunLoopDoSources0 ()
#57 0x00007fff84c79879 in __CFRunLoopRun ()
#58 0x00007fff84c7903f in CFRunLoopRunSpecific ()
#59 0x00007fff827dbc4e in RunCurrentEventLoopInMode ()
#60 0x00007fff827dba53 in ReceiveNextEventCommon ()
#61 0x00007fff827db90c in BlockUntilNextEventMatchingListInMode ()
#62 0x00007fff84238570 in _DPSNextEvent ()
#63 0x00007fff84237ed9 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#64 0x00007fff841fdb29 in -[NSApplication run] ()
#65 0x0000000112fd044d in nsAppShell::Run (this=0x100936bd0) at /files/mozilla/src/widget/src/cocoa/nsAppShell.mm:747
#66 0x00000001162e06e8 in nsAppStartup::Run (this=0x100a687f0) at /files/mozilla/src/toolkit/components/startup/src/nsAppStartup.cpp:191
#67 0x0000000100010d21 in XRE_main (argc=5, argv=0x7fff5fbfebd8, aAppData=0x100a00420) at /files/mozilla/src/toolkit/xre/nsAppRunner.cpp:3603
#68 0x0000000100001ff8 in main (argc=5, argv=0x7fff5fbfebd8) at /files/mozilla/src/browser/app/nsBrowserApp.cpp:158

Comment 7

[Bobby Holley (:bholley)]

Attachment: Remove AddScriptBlockerAndPreventAddingRunners() API and warn about script runner failures. v1

At least 2/3rds of the benefit of this patch comes from the parts that didn't go orange. I'm going to stop sitting on this one and get it landed. Flagging bz for review.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 590711 [details] [diff] [review]
Remove AddScriptBlockerAndPreventAddingRunners() API and warn about script runner failures. v1

> +        // The script shouldn't fail to add. But if somebody broke something

s/script/script runner/ and maybe have the warning mention the 100% CPU bit?

r=me

Comment 9

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> Comment on attachment 590711 [details] [diff] [review]
> maybe have the warning mention the 100% CPU bit?

I realized this wouldn't actually make sense, since we actually avoid incrementing the counter in that case. I just changed it to "expect bad things".

Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/2b0a5accb8cc

Comment 10

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/2b0a5accb8cc