Fix async DOM events from keeping link elements alive past unlink

RESOLVED FIXED in mozilla1.9

Status

()

defect
RESOLVED FIXED
11 years ago
3 months ago

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

Tracking

Trunk
mozilla1.9
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Posted patch Patch, v1 (obsolete) — Splinter Review
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.
Attachment #312153 - Flags: review?(jonas)
Assignee: nobody → bent.mozilla
Don't think this is blocking, but it'd be nice to have in order to avoid false positives in our leak efforts.
Flags: blocking1.9-
Comment on attachment 312153 [details] [diff] [review]
Patch, v1

>+  nsresult RunDOMEventWhenSafe();

Maybe FireDOMEventWhenSafe is better?
Attachment #312153 - Flags: superreview+
Attachment #312153 - Flags: review?(jonas)
Attachment #312153 - Flags: review+
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?
Attachment #312153 - Flags: approval1.9?
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 :)

Updated

11 years ago
Attachment #312153 - Flags: approval1.9? → approval1.9+
Posted patch Patch, v2 (obsolete) — Splinter Review
Sicking sez this is better.
Attachment #312153 - Attachment is obsolete: true
Attachment #312950 - Flags: superreview?(jonas)
Attachment #312950 - Flags: review?(jonas)
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.
Attachment #312950 - Flags: superreview?(jonas)
Attachment #312950 - Flags: superreview+
Attachment #312950 - Flags: review?(jonas)
Attachment #312950 - Flags: review+
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?
If it is we need to remove the scriptblocker in nsPresShell::Destroy and add it around the various ways that can call UnbindFromTree.
On windows for now, it's somewhat expected, yes.  :(
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?
Probably not, no.  I'm pretty sure we have numerous bugs on this...
(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.)
That focus event should have been deferred by nsCSSFrameConstructor::BeginUpdate calling NS_SuppressFocusEvent. Why wasn't it?
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?
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.
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.
So it seems like we need to put focus event suppression around frame tree destruction to prevent the crash regression in comment #8.
Attachment #312950 - Attachment is obsolete: true
Attachment #314456 - Attachment is obsolete: true
Attachment #314972 - Flags: superreview?(jonas)
Attachment #314972 - Flags: review?(jonas)
With missing file
Attachment #314972 - Attachment is obsolete: true
Attachment #314973 - Flags: superreview?(jonas)
Attachment #314973 - Flags: review?(jonas)
Attachment #314972 - Flags: superreview?(jonas)
Attachment #314972 - Flags: review?(jonas)
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.
Attachment #314973 - Flags: superreview?(jonas)
Attachment #314973 - Flags: superreview+
Attachment #314973 - Flags: review?(jonas)
Attachment #314973 - Flags: review+
Comment on attachment 314980 [details] [diff] [review]
Patch to check in

Passes all mochitests.
Attachment #314980 - Flags: approval1.9?
Comment on attachment 314980 [details] [diff] [review]
Patch to check in

a1.9=beltzner
Attachment #314980 - Flags: approval1.9? → approval1.9+
Fixed.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
...for the crash, if for nothing else.
Flags: in-testsuite?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.