390626 - Crash due to too much recursion in XPCWrappedNative::GetWrappedNativeOfJSObject

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

This crashes Firefox (Mac trunk debug, just updated):

function boom()
{
  var cdp = document.body.compareDocumentPosition;
  var ew = eval(window); // This creates a cross-origin wrapper!!!
  window.__proto__ = ew;
  cdp(null);
}

Comment 1

[Jesse Ruderman]

I guess this is creating a __proto__ cycle that's undetected due to the XOW, so the bug is in the __proto__ setting code for not detecting the cycle.  Why does eval(window) create a XOW, though?

Comment 2

[Jesse Ruderman]

Oh, the eval isn't needed.  |window| itself is a XOW and breaks __proto__ cycle prevention.

(Why are |window| and |document| XOWs?)

Comment 3

[Jesse Ruderman]

Attachment: testcase without eval

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #2)
> (Why are |window| and |document| XOWs?)

Because they can change principals, consider:

w = window.open(); // w is same-origin.
w.location = 'http://www.google.com'; // w is now not same-origin.

We create an XOW to ensure that we always do the right security checks.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix, v1

This should fix things: now the XOW does the cycle check, unwrapping along the way. This is a little bit complicated, since doing:

  window.__proto__ = null
  window.__proto__ = window

Does not actually create a cycle (since the first assignment disables __proto__ setting).

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix, v1.1

Oops, used the wrong variable in the JS_GetPrototype in the loop condition.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 275006 [details] [diff] [review]
Fix, v1.1

>+  JSObject *prototype = nsnull; // Initialize this to quiet GCC.

s/prototype/proto/ to match other uses of the shorthand, for brevity, and to keep the f.prototype property of function objects distant?

>+    // Ensure that this __proto__ setting didn't create a cycle. The JS
>+    // engine tries to do this, but XOWs confuse it, here we deal with
>+    // them by unwrapping each step up the prototype chain.

Full stop after "confuse it." Perhaps the new sentence starting after that should begin "So here we deal...".

r=me, nits picked.

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With nits addressed

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 10

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007123104 Minefield/3.0b3pre and the testcase. No crash on testcase - changing to Verified fixed.