Last Comment Bug 382444 - Crash [@ nsINodeInfo::Equals] with underflow event, tree stuff and removing window
: Crash [@ nsINodeInfo::Equals] with underflow event, tree stuff and removing w...
Status: VERIFIED FIXED
[sg:critical?]
: crash, testcase, verified1.8.0.13, verified1.8.1.5
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
: Neil Deakin
Mentors:
Depends on: 388583 509602
Blocks: 381120
  Show dependency treegraph
 
Reported: 2007-05-30 06:45 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-13 10:01 PDT (History)
8 users (show)
dveditz: blocking1.8.1.5+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.13+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.39 KB, text/html)
2007-05-30 06:45 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
WIP (12.44 KB, patch)
2007-05-30 11:58 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
WIP2 (20.85 KB, patch)
2007-05-31 06:35 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
possible patch (21.23 KB, patch)
2007-06-04 03:56 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
async scroll event (21.23 KB, patch)
2007-06-12 09:34 PDT, Olli Pettay [:smaug]
roc: review+
roc: superreview+
Details | Diff | Splinter Review
for 1.8 (16.28 KB, patch)
2007-06-28 08:57 PDT, Olli Pettay [:smaug]
roc: review+
roc: superreview+
dveditz: approval1.8.1.5+
dveditz: approval1.8.0.13+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2007-05-30 06:45:10 PDT
Created attachment 266610 [details]
testcase

See testcase, which crashes current trunk Mozilla builds.
It also crashes branch builds, so marking it security sensitive for now.

Talkback ID: TB32658873Y
nsINodeInfo::Equals  [mozilla/dist/include/content/nsinodeinfo.h, line 240]
nsTreeBoxObject::RowCountChanged  [mozilla/layout/xul/base/src/tree/src/nstreeboxobject.cpp, line 449]
nsBindingManager::ContentRemoved  [mozilla/content/xbl/src/nsbindingmanager.cpp, line 1341]
nsNodeUtils::ContentRemoved  [mozilla/content/base/src/nsnodeutils.cpp, line 156]
nsGenericElement::doRemoveChildAt  [mozilla/content/base/src/nsgenericelement.cpp, line 2544]
nsGenericElement::RemoveChildAt  [mozilla/content/base/src/nsgenericelement.cpp, line 2493]
nsXULElement::RemoveChildAt  [mozilla/content/xul/content/src/nsxulelement.cpp, line 941]
nsGenericElement::doRemoveChild  [mozilla/content/base/src/nsgenericelement.cpp, line 3107]
nsGenericElement::RemoveChild  [mozilla/content/base/src/nsgenericelement.cpp, line 2677]
XPCWrappedNative::CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2229]

The iframe content consists of this:
<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
<tree style="overflow: auto; display: -moz-inline-box;">
<treeitem style="overflow: scroll; display: table-cell;">
<treechildren style=" display: table-row;">
<treeitem id="a" style="display: table-cell;">
</treeitem>
</treechildren>
</treeitem>

</tree>

<script xmlns="http://www.w3.org/1999/xhtml">
function doe() {
document.getElementById('a').parentNode.removeChild(document.getElementById('a'));
}
setTimeout(doe, 100);
document.addEventListener('underflow', function(e) {window.frameElement.parentNode.removeChild(window.frameElement) }, true);
window.addEventListener('underflow', function(e) {window.frameElement.parentNode.removeChild(window.frameElement) }, true);
</script>
</window>
Comment 1 Olli Pettay [:smaug] 2007-05-30 11:58:04 PDT
Created attachment 266641 [details] [diff] [review]
WIP

I'll still have to figure out what to do with scroll event dispatching.
Comment 2 Olli Pettay [:smaug] 2007-05-31 06:35:51 PDT
Created attachment 266742 [details] [diff] [review]
WIP2
Comment 3 Olli Pettay [:smaug] 2007-06-04 03:56:33 PDT
Created attachment 267127 [details] [diff] [review]
possible patch

So the patch moves attribute setting and event dispatching to happen 
a bit later, and those are protected by nsWeakFrames.
nsWeakFrames are used only in FullScrollbarsUpdate(...) and nsScrollEventBatch.
Scroll event dispatching happens still synchronously.
Using nsScrollEventBatch is a bit ugly, and to make sure there is always 
one object in the stack, I added some debug code .
Comments? And maybe even a review ;)
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-06-11 22:09:36 PDT
Can we not dispatch these events asynchronously?
Comment 5 Olli Pettay [:smaug] 2007-06-12 09:34:44 PDT
Created attachment 268095 [details] [diff] [review]
async scroll event
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-06-12 16:28:18 PDT
+    if (!FullScrollbarsUpdate(PR_FALSE)) {
+      return PR_FALSE;
+    }
   }
 
   mReflowCallbackPosted = PR_FALSE;

Why are you returning PR_FALSE here?

Can't you move mReflowCallbackPosted = PR_FALSE; up above, say to the start of the method so you don't need this early return?

+    if (FullScrollbarsUpdate(PR_FALSE)) {
+      MarkDirtyIfSelect();
+    }
     return NS_OK;

Can't you move MarkDirtyIfSelect up above so this conditional isn't needed? (Can tree frames actually be built for a select anyway? What is MarkDirtyIfSelect for?)

+  if (FullScrollbarsUpdate(needsInvalidation)) {
+    MarkDirtyIfSelect();
+  }

Ditto

With those changes, FullScrollbarsUpdate doesn't need to return a value. 
Comment 7 Olli Pettay [:smaug] 2007-06-12 23:12:50 PDT
(In reply to comment #6)
> +    if (!FullScrollbarsUpdate(PR_FALSE)) {
> +      return PR_FALSE;
> +    }
>    }
> 
>    mReflowCallbackPosted = PR_FALSE;
> 
> Why are you returning PR_FALSE here?

Because the method is returning PR_FALSE anyway.

> 
> Can't you move mReflowCallbackPosted = PR_FALSE; up above, say to the start of
> the method so you don't need this early return?
> 

No, because I want to make sure that new callback isn't posted while 
one is being handled (to prevent endless loops or whatever). 
Calling FullScrollbarsUpdate may in theory 
post new callbacks if mReflowCallbackPosted is PR_FALSE.

> +    if (FullScrollbarsUpdate(PR_FALSE)) {
> +      MarkDirtyIfSelect();
> +    }
>      return NS_OK;
> 
> Can't you move MarkDirtyIfSelect up above so this conditional isn't needed?
> (Can tree frames actually be built for a select anyway? What is
> MarkDirtyIfSelect for?)

MarkDirtyIfSelect is for xbl form controls (which haven't ever 
happened), so perhaps I could remove it, but in a different bug where 
also other code related to xbl form control is removed.
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-06-13 02:49:03 PDT
Comment on attachment 268095 [details] [diff] [review]
async scroll event

OK!
Comment 9 Olli Pettay [:smaug] 2007-06-14 01:43:44 PDT
Checked in.
Comment 10 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-06-14 07:07:51 PDT
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070614 Minefield/3.0a6pre
Comment 11 Daniel Veditz [:dveditz] 2007-06-26 17:13:15 PDT
I crash on a deleted nsTreeBodyFrame with this testcase in both trunk (pre-fix) and branch.
Comment 12 Olli Pettay [:smaug] 2007-06-28 08:57:45 PDT
Created attachment 270188 [details] [diff] [review]
for 1.8
Comment 13 Olli Pettay [:smaug] 2007-06-28 08:58:33 PDT
(In reply to comment #12)
> Created an attachment (id=270188) [details]
> for 1.8
> 
Fixes also bug 381120.
Comment 14 Daniel Veditz [:dveditz] 2007-07-02 10:34:40 PDT
Comment on attachment 270188 [details] [diff] [review]
for 1.8

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers
Please land this soon, it looks fairly different from the trunk patch so we don't benefit from any trunk nightly testing. qawanted: we'll have to beat on the branch version pretty good once fixed.
Comment 15 Olli Pettay [:smaug] 2007-07-02 13:12:34 PDT
Checked in to branches.
The patch isn't that different. Trunk's xul:tree just has more features
(for example horiz/vertical scrolling) to cover.
But good testing is still needed, I agree.
Comment 16 Carsten Book [:Tomcat] 2007-07-04 12:05:56 PDT
verified fixed 1.8.1.5 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.5pre) Gecko/20070704 BonEcho/2.0.0.5pre ID:2007070403 no crash during tests with the testcase from this bug, so adding verified keyword.

The only thing i noticed during the tests on branch is that with this testcase i get :

Error: window.frameElement has no properties
Source File: data:application/vnd.mozilla.xul+xml;charset=utf-8,%3Cwindow%20xmlns%3D%22http%3A//www.mozilla.org/keymaster/gatekeeper/there.is.only.xul%22%3E%0A%3Ctree%20style%3D%22overflow%3A%20auto%3B%20display%3A%20-moz-inline-box%3B%22%3E%0A%3Ctreeitem%20style%3D%22overflow%3A%20scroll%3B%20display%3A%20table-cell%3B%22%3E%0A%3Ctreechildren%20style%3D%22%20display%3A%20table-row%3B%22%3E%0A%3Ctreeitem%20id%3D%22a%22%20style%3D%22display%3A%20table-cell%3B%22%3E%0A%3C/treeitem%3E%0A%3C/treechildren%3E%0A%3C/treeitem%3E%0A%0A%3C/tree%3E%0A%0A%3Cscript%20xmlns%3D%22http%3A//www.w3.org/1999/xhtml%22%3E%0Afunction%20doe%28%29%20%7B%0Adocument.getElementById%28%27a%27%29.parentNode.removeChild%28document.getElementById%28%27a%27%29%29%3B%0A%7D%0AsetTimeout%28doe%2C%20100%29%3B%0Adocument.addEventListener%28%27underflow%27%2C%20function%28e%29%20%7Bwindow.frameElement.parentNode.removeChild%28window.frameElement%29%20%7D%2C%20true%29%3B%0Awindow.addEventListener%28%27underflow%27%2C%20function%28e%29%20%7Bwindow.frameElement.parentNode.removeChild%28window.frameElement%29%20%7D%2C%20true%29%3B%0A%3C/script%3E%0A%3C/window%3E
Line: 17

Error: [Exception... "Unexpected error arg 0 [nsIDOMWindowInternal.frameElement]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: data:application/vnd.mozilla.xul+xml;charset=utf-8,%3Cwindow%20xmlns%3D%22http%3A//www.mozilla.org/keymaster/gatekeeper/there.is.only.xul%22%3E%0A%3Ctree%20style%3D%22overflow%3A%20auto%3B%20display%3A%20-moz-inline-box%3B%22%3E%0A%3Ctreeitem%20style%3D%22overflow%3A%20scroll%3B%20display%3A%20table-cell%3B%22%3E%0A%3Ctreechildren%20style%3D%22%20display%3A%20table-row%3B%22%3E%0A%3Ctreeitem%20id%3D%22a%22%20style%3D%22display%3A%20table-cell%3B%22%3E%0A%3C/treeitem%3E%0A%3C/treechildren%3E%0A%3C/treeitem%3E%0A%0A%3C/tree%3E%0A%0A%3Cscript%20xmlns%3D%22http%3A//www.w3.org/1999/xhtml%22%3E%0Afunction%20doe%28%29%20%7B%0Adocument.getElementById%28%27a%27%29.parentNode.removeChild%28document.getElementById%28%27a%27%29%29%3B%0A%7D%0AsetTimeout%28doe%2C%20100%29%3B%0Adocument.addEventListener%28%27underflow%27%2C%20function%28e%29%20%7Bwindow.frameElement.parentNode.removeChild%28window.frameElement%29%20%7D%2C%20true%29%3B%0Awindow.addEventListener%28%27underflow%27%2C%20function%28e%29%20%7Bwindow.frameElement.parentNode.removeChild%28window.frameElement%29%20%7D%2C%20true%29%3B%0A%3C/script%3E%0A%3C/window%3E :: anonymous :: line 18"  data: no]
Source File: data:application/vnd.mozilla.xul+xml;charset=utf-8,%3Cwindow%20xmlns%3D%22http%3A//www.mozilla.org/keymaster/gatekeeper/there.is.only.xul%22%3E%0A%3Ctree%20style%3D%22overflow%3A%20auto%3B%20display%3A%20-moz-inline-box%3B%22%3E%0A%3Ctreeitem%20style%3D%22overflow%3A%20scroll%3B%20display%3A%20table-cell%3B%22%3E%0A%3Ctreechildren%20style%3D%22%20display%3A%20table-row%3B%22%3E%0A%3Ctreeitem%20id%3D%22a%22%20style%3D%22display%3A%20table-cell%3B%22%3E%0A%3C/treeitem%3E%0A%3C/treechildren%3E%0A%3C/treeitem%3E%0A%0A%3C/tree%3E%0A%0A%3Cscript%20xmlns%3D%22http%3A//www.w3.org/1999/xhtml%22%3E%0Afunction%20doe%28%29%20%7B%0Adocument.getElementById%28%27a%27%29.parentNode.removeChild%28document.getElementById%28%27a%27%29%29%3B%0A%7D%0AsetTimeout%28doe%2C%20100%29%3B%0Adocument.addEventListener%28%27underflow%27%2C%20function%28e%29%20%7Bwindow.frameElement.parentNode.removeChild%28window.frameElement%29%20%7D%2C%20true%29%3B%0Awindow.addEventListener%28%27underflow%27%2C%20function%28e%29%20%7Bwindow.frameElement.parentNode.removeChild%28window.frameElement%29%20%7D%2C%20true%29%3B%0A%3C/script%3E%0A%3C/window%3E
Line: 18

Error: [Exception... "'Permission denied to get property HTMLIFrameElement.parentNode' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "<unknown>"  data: no]

but i think that is expected, or ? 
Comment 17 Carsten Book [:Tomcat] 2007-07-04 12:18:47 PDT
as little notice, the difference between branch and trunk is that i get 3 error messages in the error console in branch (comment #16) and only one in trunk Error: window.frameElement has no properties
Source File: data:application/vnd.mozilla.xul+xml;charset=utf-8,%3Cwindow%20xmlns%3D%22http%3A//www.mozilla.org/keymaster/gatekeeper/there.is.only.xul%22%3E%0A%3Ctree%20style%3D%22overflow%3A%20auto%3B%20display%3A%20-moz-inline-box%3B%22%3E%0A%3Ctreeitem%20style%3D%22overflow%3A%20scroll%3B%20display%3A%20table-cell%3B%22%3E%0A%3Ctreechildren%20style%3D%22%20display%3A%20table-row%3B%22%3E%0A%3Ctreeitem%20id%3D%22a%22%20style%3D%22display%3A%20table-cell%3B%22%3E%0A%3C/treeitem%3E%0A%3C/treechildren%3E%0A%3C/treeitem%3E%0A%0A%3C/tree%3E%0A%0A%3Cscript%20xmlns%3D%22http%3A//www.w3.org/1999/xhtml%22%3E%0Afunction%20doe%28%29%20%7B%0Adocument.getElementById%28%27a%27%29.parentNode.removeChild%28document.getElementById%28%27a%27%29%29%3B%0A%7D%0AsetTimeout%28doe%2C%20100%29%3B%0Adocument.addEventListener%28%27underflow%27%2C%20function%28e%29%20%7Bwindow.frameElement.parentNode.removeChild%28window.frameElement%29%20%7D%2C%20true%29%3B%0Awindow.addEventListener%28%27underflow%27%2C%20function%28e%29%20%7Bwindow.frameElement.parentNode.removeChild%28window.frameElement%29%20%7D%2C%20true%29%3B%0A%3C/script%3E%0A%3C/window%3E
Line: 17
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-07-18 14:34:56 PDT
May have caused 388583
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-07-18 15:42:28 PDT
The problem seems to be that nsNativeScrollbarFrame::Hookup is called from nsNativeScrollbarFrame::GetPrefSize. The first time it runs it interposes itself as the scrollbar mediator: it keeps a reference to the old scrollbar mediator, makes itself the new scrollbar mediator, and forwards messages from the scrollbar to the old mediator.

But this patch is setting the scrollbar mediator after GetPrefSize has run for the first time. So nsNativeScrollbarFrame thinks there is no mediator and drops all the events on the floor.
Comment 20 Stephen Donner [:stephend] 2007-08-21 15:53:33 PDT
I verified that the crash (only) no longer occurs under Thunderbird: version 1.5.0.13 (20070809).  Replacing fixed1.5.0.13 with verified1.5.0.13.
Comment 21 Bob Clary [:bc:] 2009-04-24 10:47:26 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/a53ccd54282e

Note You need to log in before you can comment on or make changes to this bug.