Crash due to too much recursion in XPCWrappedNative::GetWrappedNativeOfJSObject

VERIFIED FIXED

Status

()

Core
XPConnect
--
critical
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Jesse Ruderman, Assigned: mrbkap)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

Trunk
x86
Mac OS X
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
Created attachment 274926 [details]
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);
}
Flags: blocking1.9?
(Reporter)

Comment 1

11 years ago
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?
(Reporter)

Comment 2

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

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

Comment 3

11 years ago
Created attachment 274927 [details]
testcase without eval
Attachment #274926 - Attachment is obsolete: true
(Assignee)

Comment 4

11 years ago
(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.
(Assignee)

Comment 5

11 years ago
Created attachment 275005 [details] [diff] [review]
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).
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #275005 - Flags: superreview?(jst)
Attachment #275005 - Flags: review?(brendan)
(Assignee)

Comment 6

11 years ago
Created attachment 275006 [details] [diff] [review]
Fix, v1.1

Oops, used the wrong variable in the JS_GetPrototype in the loop condition.
Attachment #275005 - Attachment is obsolete: true
Attachment #275006 - Flags: superreview?(jst)
Attachment #275006 - Flags: review?(brendan)
Attachment #275005 - Flags: superreview?(jst)
Attachment #275005 - Flags: review?(brendan)

Updated

11 years ago
Flags: blocking1.9? → blocking1.9+
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
Attachment #275006 - Flags: review?(brendan) → review+
(Assignee)

Comment 8

11 years ago
Created attachment 275019 [details] [diff] [review]
With nits addressed
Attachment #275006 - Attachment is obsolete: true
Attachment #275019 - Flags: superreview?(jst)
Attachment #275019 - Flags: review+
Attachment #275006 - Flags: superreview?(jst)

Updated

11 years ago
Attachment #275019 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 9

11 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.