425551 - Fix async DOM events from keeping link elements alive past unlink

Status: RESOLVED FIXED

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

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

DEBUG_CC builds are warning about link elements that live past Unlink and they're due to async events being fired from UnbindFromTree. This patch uses the script blocker/runner to fire the event instead and guards against other script running during Unlink.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Don't think this is blocking, but it'd be nice to have in order to avoid false positives in our leak efforts.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 312153 [details] [diff] [review]
Patch, v1

>+  nsresult RunDOMEventWhenSafe();

Maybe FireDOMEventWhenSafe is better?

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 312153 [details] [diff] [review]
Patch, v1

Requesting approval to check this in.

Jonas, what sorts of bad things could happen without this nsAutoScriptBlocker in unlink?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We could end up running script in places where it's not safe to do so, causing exploitable crashes.

However, I don't know of any places in UnbindFromTree that would cause script to run. If there are such places we likely won't immediately stop it with this patch since so far very few things pay attention to nsContentUtils::IsSafeToRunScript.

So this is a step on the way. All of this is new infrastructure so thread carefully :)

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

Sicking sez this is better.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 312950 [details] [diff] [review]
Patch, v2

>diff -NprU8 mozilla.fcbee8b3e9d0/content/xbl/src/nsXBLBinding.cpp mozilla/content/xbl/src/nsXBLBinding.cpp
>@@ -342,16 +342,17 @@ nsXBLBinding::InstallAnonymousContent(ns
>   // aElement.
>   // (2) The children's parent back pointer should not be to this synthetic root
>   // but should instead point to the enclosing parent element.
>   nsIDocument* doc = aElement->GetCurrentDoc();
>   PRBool allowScripts = AllowScripts();
> 
>   PRUint32 childCount = aAnonParent->GetChildCount();
>   for (PRUint32 i = 0; i < childCount; i++) {
>+    nsAutoScriptBlocker scriptBlocker;
>     nsIContent *child = aAnonParent->GetChildAt(i);
>     child->UnbindFromTree();

I think you should move the scriptblocker to outside of the for-loop.

>diff -NprU8 mozilla.fcbee8b3e9d0/layout/base/nsPresShell.cpp mozilla/layout/base/nsPresShell.cpp
>@@ -1698,18 +1698,21 @@ PresShell::Destroy()
>   // apparently frame destruction sometimes spins the event queue when
>   // plug-ins are involved(!).
>   mReflowEvent.Revoke();
> 
>   CancelAllPendingReflows();
>   CancelPostedReflowCallbacks();
> 
>   // Destroy the frame manager. This will destroy the frame hierarchy
>-  mFrameConstructor->WillDestroyFrameTree();
>-  FrameManager()->Destroy();
>+  {
>+    nsAutoScriptBlocker scriptBlocker;
>+    mFrameConstructor->WillDestroyFrameTree();
>+    FrameManager()->Destroy();
>+  }

I think you should remove the scoping here so that the rest of the function is covered too.

r/sr=me with that.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2, unbitrotted

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So I landed and had to back out. The test_emptytext.xul test was timing out. The source seems to be us getting lots and lots of the following assertion:

 ASSERTION: How did we get here if it's not safe to run scripts?: 'Error', file d:/moz/checkintree/mozilla/layout/base/nsPresShell.cpp, line 5593

I.e. we end up trying in PresShell::HandleEvent when there are scriptblockers active.

Here's one stack where it happens:

xpcom_core.dll!NS_DebugBreak_P(unsigned int aSeverity=0x00000001, const char * aStr=0x020f2d50, const char * aExpr=0x020f2d48, const char * aFile=0x020f2d10, int aLine=0x000015d9)  Line 271 C++
gklayout.dll!PresShell::HandleEvent(nsIView * aView=0x04d16178, nsGUIEvent * aEvent=0x0012ee40, nsEventStatus * aEventStatus=0x0012ec3c)  Line 5593 + 0x1c bytes  C++
gklayout.dll!nsViewManager::HandleEvent(nsView * aView=0x04d16178, nsPoint aPoint={...}, nsGUIEvent * aEvent=0x0012ee40, int aCaptured=0x00000000)  Line 1385 C++
gklayout.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x0012ee40, nsEventStatus * aStatus=0x0012ed84)  Line 1337 + 0x22 bytes C++
gklayout.dll!HandleEvent(nsGUIEvent * aEvent=0x0012ee40)  Line 171  C++
gkwidget.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0012ee40, nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 973 + 0xc bytes  C++
gkwidget.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x0012ee40)  Line 994 C++
gkwidget.dll!nsWindow::DispatchFocus(unsigned int aEventType=0x00000069, int isMozWindowTakingFocus=0x00000001)  Line 6032 + 0x11 bytes C++
gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=0x00000007, unsigned int wParam=0x003d05ec, long lParam=0x00000000, long * aRetValue=0x0012f274)  Line 4530 + 0x17 bytes C++
gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x006205a6, unsigned int msg=0x00000007, unsigned int wParam=0x003d05ec, long lParam=0x00000000)  Line 1188 + 0x1d bytes  C++
user32.dll!7e418734()   
[Frames below may be incorrect and/or missing, no symbols loaded for user32.dll]  
user32.dll!7e418816()   
user32.dll!7e41b4c0()   
user32.dll!7e41b50c()   
ntdll.dll!7c90eae3()  
user32.dll!7e41daf6()   
gkwidget.dll!nsWindow::Destroy()  Line 1491 + 0x10 bytes  C++
gklayout.dll!nsView::~nsView()  Line 274  C++
gklayout.dll!nsView::`scalar deleting destructor'()  + 0xf bytes  C++
gklayout.dll!nsIView::Destroy()  Line 314 + 0x21 bytes  C++
gklayout.dll!nsFrame::Destroy()  Line 510 C++
gklayout.dll!nsSplittableFrame::Destroy()  Line 74  C++
gklayout.dll!nsContainerFrame::Destroy()  Line 300  C++
gklayout.dll!ViewportFrame::Destroy()  Line 69  C++
gklayout.dll!nsFrameManager::Destroy()  Line 284  C++
gklayout.dll!PresShell::Destroy()  Line 1713  C++


So we from PresShell::Destroy end up in nsWindow::Destroy which spins the event loop. Is this expected?

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If it is we need to remove the scriptblocker in nsPresShell::Destroy and add it around the various ways that can call UnbindFromTree.

Comment 10

[Boris Zbarsky [:bzbarsky]]

On windows for now, it's somewhat expected, yes.  :(

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Do does that mean we're also fine with dealing with whatever that script can run? I.e. we're fine with things potentially going away during the ::Destroy callchain?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Probably not, no.  I'm pretty sure we have numerous bugs on this...

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #8)
> So we from PresShell::Destroy end up in nsWindow::Destroy which spins the event
> loop. Is this expected?

AFAIK, it doesn't actually spin the event loop, but possibly fires a focus event.
That is the reason why we have nsIFocusEventSuppressor.h. :(

(I really don't understand why Windows does that.)

Comment 14

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

That focus event should have been deferred by nsCSSFrameConstructor::BeginUpdate calling NS_SuppressFocusEvent. Why wasn't it?

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The event suppression doesn't seem to work that well though since when we really block it (using the patch in this bug) mochitests start failing :(

Or maybe this patch is blocking it the wrong way somehow?

Comment 16

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

What is supposed to be calling NS_SuppressFocusEvent or nsCSSFrameConstructor::BeginUpdate from PresShell::Destroy? I don't see it. You have an nsAutoScriptBlocker there, but that doesn't suppress focus changes.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, this patch isn't meant to fix the focus-problem. It just highlighted the fact that we're getting to nsPresShell::HandleEvent at a point where we don't seem to expect to.

Comment 18

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

So it seems like we need to put focus event suppression around frame tree destruction to prevent the crash regression in comment #8.

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Trimmed down patch that avoids layout problems

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Trimmed down patch that avoids layout problems

With missing file

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 314973 [details] [diff] [review]
Trimmed down patch that avoids layout problems

>+class mozAutoDocUpdate
>+{
>+public:
>+  mozAutoDocUpdate(nsIDocument* aDocument, nsUpdateType aUpdateType,
>+                   PRBool aNotify) :
>+    mDocument(aNotify ? aDocument : nsnull),
>+    mUpdateType(aUpdateType)
>+  {
>+    if (mDocument) {
>+      mDocument->BeginUpdate(mUpdateType);
>+    }
>+    else {
>+      nsContentUtils::AddScriptBlocker();
>+    }
>+  }
>+
>+  ~mozAutoDocUpdate()
>+  {
>+    if (mDocument) {
>+      mDocument->EndUpdate(mUpdateType);
>+    }
>+    else {
>+      nsContentUtils::RemoveScriptBlocker();
>+    }
>+  }

Unfortunately I don't think we're ready to add the script blockers in the |else| statements here. It will cause script blockers to exist while mutation events fire in orphaned trees, which is bad.

r/sr=me with those removed.

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch to check in

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 314980 [details] [diff] [review]
Patch to check in

Passes all mochitests.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 314980 [details] [diff] [review]
Patch to check in

a1.9=beltzner

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed.

Comment 26

[Jeff Walden [:Waldo]]

...for the crash, if for nothing else.