635835 - Crash with nsTreeWalker::NextNode setting currentNode as JS object from chrome

Status: RESOLVED FIXED

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

Description

[Alex Vincent [:WeirdAl]]

Attachment: maximal testcase as XPI

Steps to reproduce:
(1) Take a debug build of FF4 trunk or beta 11
(2) mkdir crash-profile
(3) Download the attachment as crash-proxy-test.xpi
(4) dist/bin/firefox --no-remote -profile crash-profile/ crash-proxy-test.xpi 
(5) Install the XPI as directed, and after restarting, close the browser.
(6) dist/bin/firefox --no-remote -profile crash-profile/ --chrome chrome://editortest/content/test_DOM_bindings.xul

Expected results:  No crash.
Actual results:  Crash at nsINode::GetFirstChild, this == null.

Stack trace coming in a moment.  I'll spend the next few days reducing this testcase.

Basically, I used a bit of JS-based proxy code to wrap a DOM document.  Then I called this code, where the DOM document is called "doc":

  // Remove all white-space.
  function filter(aNode) {
    return (!(/\S/.test(aNode.nodeValue))) ? FILTER_ACCEPT : FILTER_SKIP;
  }
  var walker = doc.createTreeWalker(doc, C_i.nsIDOMNodeFilter.SHOW_TEXT, filter, true);
  // Skip the document node, since they can't have text children.
  while (walker.nextNode()) {
    dump("walker.currentNode: " + walker.currentNode.nodeName + "\n");
    var parent = walker.currentNode.parentNode;
    parent.removeChild(walker.currentNode);
    dump("parent: " + parent.nodeName + "\n\n");
    debugger;
    walker.currentNode = parent;
  }

I think it tried to set walker.currentNode to the proxied parent node, that somehow the JS-proxied parent node made it all the way into the native code.  I see one dump("parent: " ...) call succeed, and then the null dereference.

Comment 1

[Mike Shaver (:shaver emeritus)]

per reporter request

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: stack trace

On the security-sensitive flag:  Both gal and Waldo asked me to file it as such; I simply forgot to check the box.

Comment 3

[Alex Vincent [:WeirdAl]]

(In reply to comment #0)
> I think it tried to set walker.currentNode to the proxied parent node, that
> somehow the JS-proxied parent node made it all the way into the native code.

Stepping through on Windows, I now see my theory was correct:  at nsTreeWalker::SetCurrentNode(nsIDOMNode * aCurrentNode), aCurrentNode is a [nsXPTCStubBase].

Comment 4

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

I suspect this is neither proxy related or a security problem.

It is expected that we'll get to nsTreeWalker::SetCurrentNode with the proxy object as aCurrentNode. You can pass any (non-xpcom) js-object as node and we'll happily wrap it. However when we QI on line 150, the result of the QI should be null. An alternative way to get the same result is to just do

walker.currentNode = {};

Anyhow, it appears that some of the walking functions aren't able to deal with mCurrentNode being null, which makes sense as that normally shouldn't happen.

So what we should do is to nullcheck the result of the QI before assigning into mCurrentNode in nsTreeWalker::SetCurrentNode.

Comment 5

[Alex Vincent [:WeirdAl]]

Jonas's guess (about null-checking the QI) is right on the money, according to a patch I tested locally.

The only thing I don't understand is why the QI failed.  The way I wrote my proxied QI, it calls on all the QI targets it can, including the original object's.  Unless the native do_QueryInterface call itself (or XPConnect code) decided, "This interface isn't scriptable, so I'm not going to call the scripted object's QueryInterface method," it should have worked.

(I can understand why we wouldn't call QI with a non-scriptable interface.  Why have a nsCOMPtr to a non-scriptable interface for a scriptable object, since you can't call those methods and expect them to work?  Still, it'd be nice to have that confirmed.)

Comment 6

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

Yes, the XPConnect QI never calls into the wrapped object for nonscripatable interfaces.

Comment 7

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

Please renominate if this hits sg:crit, otherwise we'll be happy to process your approval, but won't block on this.

Comment 8

[Boris Zbarsky [:bzbarsky]]

In particular, non-scriptable interfaces are not exposed to script, period.  You can't QI an object to them from script, and you can't implement them in script.

Comment 9

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

Opening this up since it's just a null-dereference.

Comment 10

[Alex Vincent [:WeirdAl]]

I'm working on a patch for this - with a couple details answered here, I can have it tonight for review.

(1) Which exception code would people prefer?  I was thinking NS_ERROR_ILLEGAL_VALUE, but DOM code usually says NS_ERROR_DOM_NOT_SUPPORTED_ERR.
(2) Are there "chrome crashtests", and if so, how do I specify a HTML file is one?  (I tried writing it as an ordinary crashtest, and it didn't crash because nsContentUtils::CheckSameOrigin rejected it before we got to the buggy behavior.)

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

NOT_SUPPORTED_ERR: Raised if an attempt is made to set currentNode to null.

http://www.w3.org/TR/DOM-Level-2-Traversal-Range/traversal.html#Traversal-TreeWalker-currentNode

Comment 12

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

I thought about putting this check before the CheckSameOrigin call, where we check that aCurrentNode is not null.  I don't have a strong opinion on that, though.

Comment 13

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

Comment on attachment 514409 [details] [diff] [review]
patch, v1

Would prefer to write a simple crash-test rather than a chrome test, as, well, that's what they're for :-)

Comment 14

[Alex Vincent [:WeirdAl]]

Comment on attachment 514409 [details] [diff] [review]
patch, v1

Requesting approval 2.0:  This fixes a crash in nsTreeWalker, which I believe only chrome code (well, maybe proxy code, but I doubt it) can reach.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 514409 [details] [diff] [review]
patch, v1

Seems to me that this can change behavior in some case from returning null to throwing an exception, which could break scripts in extensions etc. Given that, and the fact that this crash hasn't been seen much (if at all) before, and that it's a non-exploitable crash, I'd prefer this to land after 2.0. So approval2.0-

Comment 16

[(no longer active)]

http://hg.mozilla.org/projects/cedar/rev/3803428ad641

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/3803428ad641