382444 - Crash [@ nsINodeInfo::Equals] with underflow event, tree stuff and removing window

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Martijn Wargers (dead)]

Attachment: 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][[e-mail]]]

Attachment: WIP

I'll still have to figure out what to do with scroll event dispatching.

Comment 2

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

Attachment: WIP2

Comment 3

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

Attachment: 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)]

Can we not dispatch these events asynchronously?

Comment 5

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

Attachment: async scroll event

Comment 6

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

+    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][[e-mail]]]

(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)]

Comment on attachment 268095 [details] [diff] [review]
async scroll event

OK!

Comment 9

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

Checked in.

Comment 10

[Martijn Wargers (dead)]

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]]

I crash on a deleted nsTreeBodyFrame with this testcase in both trunk (pre-fix) and branch.

Comment 12

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

Attachment: for 1.8

Comment 13

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

(In reply to comment #12)
> Created an attachment (id=270188) [details]
> for 1.8
> 
Fixes also bug 381120.

Comment 14

[Daniel Veditz [:dveditz]]

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][[e-mail]]]

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]]

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]]

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)]

May have caused 388583

Comment 19

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

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] Not actively reading bugmail]

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] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/a53ccd54282e