Last Comment Bug 750307 - "Assertion failure: isBoolean(),"
: "Assertion failure: isBoolean(),"
Status: VERIFIED FIXED
[js:p1:fx15]
: assertion, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: mozilla16
Assigned To: Jason Orendorff [:jorendorff]
: Anthony Hughes (:ashughes) [GFX][QA][Mentor]
Mentors:
Depends on: 764688 765156
Blocks: jsfunfuzz 673188 CVE-2012-1956
  Show dependency treegraph
 
Reported: 2012-04-30 09:07 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2012-10-05 12:50 PDT (History)
17 users (show)
gary: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
16+
fixed


Attachments
stack (6.37 KB, text/plain)
2012-04-30 09:07 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
v1 (8.66 KB, patch)
2012-06-06 12:00 PDT, Jason Orendorff [:jorendorff]
jwalden+bmo: review+
Details | Diff | Review
v2 - same as v1, but fix a test (9.74 KB, patch)
2012-06-13 01:16 PDT, Jason Orendorff [:jorendorff]
jorendorff: review+
Details | Diff | Review
ESR attempt -- undoes part of the ESR extra bit (7.92 KB, patch)
2012-10-04 19:35 PDT, David Mandelin [:dmandelin]
no flags Details | Diff | Review

Description Gary Kwong [:gkw] [:nth10sd] 2012-04-30 09:07:17 PDT
Created attachment 619588 [details]
stack

Math.tan(
    Object.defineProperty(
        evalcx("new RegExp"),
        "ignoreCase",
        {e:true}
    )
)

asserts js debug shell on m-c changeset cfaf90b22fc3 without any CLI arguments at Assertion failure: isBoolean(),

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   78732:61dd23c012ee
user:        Chris Leary
date:        Fri Oct 07 03:04:00 2011 -0700
summary:     Bug 673188: Compile regexps lazily. (r=mrbkap)
Comment 1 Jason Orendorff [:jorendorff] 2012-05-21 16:44:51 PDT
Hmm. Having a little trouble tracking this one down actually. Taking.
Comment 2 Jason Orendorff [:jorendorff] 2012-05-22 16:15:23 PDT
This should have been obvious to me at the outset. Not sure what I was thinking.

1. For cross-compartment wrappers, Object.defineProperty ultimately calls JS_DefinePropertyById.

2. JS_DefinePropertyById allows the redefinition of non-writable non-configurable properties, contrary to ES5. In particular it does not do all the checking that obj_DefineProperty does when the target object is native.

3. RegExpObject::toString assumes the values of the "ignoreCase" non-writable non-configurable property can't be changed.
Comment 3 Jason Orendorff [:jorendorff] 2012-05-22 16:17:42 PDT
To break it down a little bit:

let a = evalcx("/x/");   // make a cross-compartment wrapper around a RegExp object
Object.defineProperty(a, "ignoreCase", {value: undefined});  // see comment 2
a.toString();  // trip an assertion that ignoreCase has a boolean value
Comment 4 Jason Orendorff [:jorendorff] 2012-06-06 08:19:41 PDT
My first attempt was to make JS_Define* respect the invariants.
This was doomed because the JS_Define* functions are the APIs resolve hooks are supposed to use. We'd get call stacks like:

   some kind of lookup
   -> resolve hook
   -> JS_DefineProperty
   -> do a lookup to check the invariants
   -> resolve hook
      ...

The resolve machinery has some goofy hash table to prevent runaway recursion in this case, but it still causes problems. I think causing JS_Define* to do lookups is too much of an API change for Aurora.

A more modest change is to change wrappers so that instead of using JS_DefinePropertyById directly, they enforce the invariants. Pursuing that now.
Comment 5 Jason Orendorff [:jorendorff] 2012-06-06 12:00:22 PDT
Created attachment 630656 [details] [diff] [review]
v1
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-08 17:26:57 PDT
Comment on attachment 630656 [details] [diff] [review]
v1

Review of attachment 630656 [details] [diff] [review]:
-----------------------------------------------------------------

The property definition algorithms are complicated enough that I'm not especially confident I haven't missed anything here, but I think you might have covered the bases.

::: js/src/jit-test/tests/basic/bug750307.js
@@ +1,5 @@
> +var g = newGlobal('new-compartment');
> +var a = g.RegExp("x");
> +try {
> +    Object.defineProperty(a, "ignoreCase", {value: undefined});
> +} catch (exc) {}

assertEq(exc instanceof g.TypeError, true); // it's g.TypeError and not TypeError, right?  And an assert verifying that an error was actually thrown, too.
Comment 7 Jason Orendorff [:jorendorff] 2012-06-11 14:42:16 PDT
Good idea.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7166a68a7994
Comment 8 Phil Ringnalda (:philor) 2012-06-11 18:37:09 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/65c7255e9d15 for the "browser_bug645699.js | an unexpected uncaught JS exception reported through window.onerror - TypeError: can't redefine non-configurable property 'location' at http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/bug645699.html:12" etc. in https://tbpl.mozilla.org/php/getParsedLog.php?id=12567825&tree=Mozilla-Inbound and friends.
Comment 9 Jason Orendorff [:jorendorff] 2012-06-12 11:59:25 PDT
Thanks for the backout. Looks like we should just change the test, though. I'll push to try in a minute.
Comment 10 Jason Orendorff [:jorendorff] 2012-06-12 12:15:46 PDT
https://tbpl.mozilla.org/?tree=Try&rev=d87a33f7beb0
Comment 11 Gary Kwong [:gkw] [:nth10sd] 2012-06-12 23:42:59 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #10)
> https://tbpl.mozilla.org/?tree=Try&rev=d87a33f7beb0

Try looks green - should be good to land! \o/
Comment 12 Phil Ringnalda (:philor) 2012-06-12 23:52:10 PDT
That must have been intended for some other bug, since every mochitest-oth which has run has a failure in the same test (plus, what's pushed to try isn't even in this bug, so checkin-needed would just reland the same thing that already bounced).
Comment 13 Jason Orendorff [:jorendorff] 2012-06-13 01:15:46 PDT
It was just a botch. Here's the fixed try run.
https://tbpl.mozilla.org/?tree=Try&rev=07957da489ad
Comment 14 Jason Orendorff [:jorendorff] 2012-06-13 01:16:42 PDT
Created attachment 632595 [details] [diff] [review]
v2 - same as v1, but fix a test

Carrying forward review.
Comment 15 Jason Orendorff [:jorendorff] 2012-06-13 01:17:42 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ddb6278d1de
Comment 16 Matt Brubeck (:mbrubeck) 2012-06-13 13:29:51 PDT
https://hg.mozilla.org/mozilla-central/rev/2ddb6278d1de
Comment 17 Scoobidiver (away) 2012-07-01 00:31:49 PDT
It seems it landed in 15.a2/20120629:
http://hg.mozilla.org/releases/mozilla-aurora/rev/2f5ede40b4f5
Comment 18 Ed Morley [:emorley] 2012-07-10 02:20:51 PDT
Landed on beta as:
https://hg.mozilla.org/releases/mozilla-beta/rev/acb98d65fa59
(Bug doesn't seem to have been updated to reflect that)

However backed out of beta for M3 orange:
https://tbpl.mozilla.org/?tree=Mozilla-Beta&rev=acb98d65fa59

https://hg.mozilla.org/releases/mozilla-beta/rev/61b48c85c866
Comment 20 Mihaela Velimiroviciu (:mihaelav) 2012-08-17 07:51:10 PDT
I built Spidermonkey for the latest Firefox 15 version (beta5, Changeset	5b309ad1a88a) and run the test from comment #0. The reported assertion did not show, but instead I got the following assertion:
test750307:5: TypeError: "ignoreCase" is read-only

Is this expected?

Thank you!
Comment 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-21 16:04:09 PDT
Confirmed testcase asserts 2012-04-30 mozilla-central debug JS shell.

Does not assert:
2012-06-14 mozilla-central debug JS shell
2012-07-01 mozilla-aurora debug JS shell
Comment 22 Jason Orendorff [:jorendorff] 2012-10-02 14:18:05 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/6b471843f125

Approval granted separately in bug 756719.
Comment 23 Justin Wood (:Callek) 2012-10-02 16:27:54 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #22)
> https://hg.mozilla.org/releases/mozilla-esr10/rev/6b471843f125
> 
> Approval granted separately in bug 756719.

Had oranges in mochi-4 and mochi-oth across all platforms, backed out
https://hg.mozilla.org/releases/mozilla-esr10/rev/e78926e3cef0
Comment 24 Jason Orendorff [:jorendorff] 2012-10-04 15:35:40 PDT
Re-landed:

   https://hg.mozilla.org/releases/mozilla-esr10/rev/9708ce55a237
   https://hg.mozilla.org/releases/mozilla-esr10/rev/b3600020156d

The "part 2" patch got review and approval separately, in bug 756719.

There's no way to get a Try Server run on ESR10 at present, so I'm not as confident in these patches as I'd normally like to be, but at worst, we back them out again...
Comment 25 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-10-04 17:13:12 PDT
> but at worst, we back them out again...
At worst it is!  Backed out for Moth oranges.

https://hg.mozilla.org/releases/mozilla-esr10/rev/96931ca23431
Comment 26 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-10-04 17:43:37 PDT
I managed to only back out the second part, so here's another attempt to finish the job.

https://hg.mozilla.org/releases/mozilla-esr10/rev/d449bf8cd785
Comment 27 David Mandelin [:dmandelin] 2012-10-04 19:35:53 PDT
Created attachment 668292 [details] [diff] [review]
ESR attempt -- undoes part of the ESR extra bit

Moth seems to fail more with the extra ESR fix on than not, so this this is a more minimal version of the ESR fix. It's impossible to quickly figure out what's really going wrong or what's needed, but this may work. Currently running on try. I'll check again in the morning.
Comment 28 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-10-04 19:37:47 PDT
You can't use Try for ESR-10 (it won't build), which is kind of the root of this whole problem. You'll have to run Moth locally.
Comment 29 David Mandelin [:dmandelin] 2012-10-04 19:40:57 PDT
(In reply to Andrew McCreight [:mccr8] from comment #28)
> You can't use Try for ESR-10 (it won't build), which is kind of the root of
> this whole problem. You'll have to run Moth locally.

That is super lame. Hopefully Jason can pick it up in the morning then.
Comment 30 Jason Orendorff [:jorendorff] 2012-10-05 07:08:01 PDT
I'm working on this now.

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