926431 - Assertion failure: isObject(), at js/Value.h:1129 with ASM.js and clone

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 211337f7fb83 (threadsafe build, run with --fuzzing-safe):


var g = newGlobal();
function cloneableFunction(body) {
  'use asm';
  function _main() {}
  return _main;
}
g.f = cloneableFunction('return function(x) { return x };');
g.eval("clone(f)()")

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

This test uses the shell-only function "clone" which was once a known-unsafe function. If this function is still the only problem here, then please add it to the fuzzing-unsafe list to avoid this problem in the future.

Needinfo from Luke because this seems to involve asm.js.

Comment 3

[Luke Wagner [:luke]]

Yes, cloning asm.js functions isn't allowed.  This is prevented in normal browsing by only doing asm.js compilation for compile-and-go code (which can't be cloned), but the shell clone() function clones anyway, so it should be in the fuzzing-unsafe list.

Comment 4

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/b3d85b68449d
user:        Luke Wagner
date:        Fri Mar 15 02:29:02 2013 -0700
summary:     Bug 840282 - OdinMonkey (sr=dmandelin)

This iteration took 163.127 seconds to run.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: bug926431.patch

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 828163 [details] [diff] [review]
bug926431.patch

Thanks!

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f1dc2186141

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2f1dc2186141

Comment 9

[Ioana (away)]

Tested on Ubuntu 13.04 32bit with Firefox 28:
- 11/01 JS Shell -> ../test926431.js:warning: successfully compiled asm.js code (total compilation time 0ms)
Assertion failure: isObject(), at ../../dist/include/js/Value.h:1128
Segmentation fault (core dumped)


- 02/04 JS Shell -> ../test926431.js:warning: successfully compiled asm.js code (total compilation time 0ms)
Assertion failure: isObject(), at ../../dist/include/js/Value.h:1124
Segmentation fault (core dumped)

It seems this bug is still reproducible.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

According to comment 4 and the patch that landed on this test case (disallowing fuzzers to use clone()), this cannot happen in the browser, as cloning asm.js functions can't happen.

Comment 11

[Christian Holler (:decoder)]

(In reply to Ioana Budnar, QA [:ioana] from comment #9)
> It seems this bug is still reproducible.

It should not be reproducible with --fuzzing-safe. That's the change that we made in the patch, so we can't hit this bug anymore :)

Comment 12

[Ioana (away)]

(In reply to Christian Holler (:decoder) from comment #11)
> (In reply to Ioana Budnar, QA [:ioana] from comment #9)
> > It seems this bug is still reproducible.
> 
> It should not be reproducible with --fuzzing-safe. That's the change that we
> made in the patch, so we can't hit this bug anymore :)

I got those results with --fuzzing-safe (didn't think to mention it since comment 0 already specified that the testcase had to be run with this option).

Comment 13

[Christian Holler (:decoder)]

(In reply to Ioana Budnar, QA [:ioana] from comment #12)

> I got those results with --fuzzing-safe (didn't think to mention it since
> comment 0 already specified that the testcase had to be run with this
> option).

That is strange. I just ran the test with --fuzzing-safe and I get:

min.js:8:0 ReferenceError: clone is not defined

That's exactly what would be expected. Without --fuzzing-safe, I get the crash. Can you double check that you didn't forget that flag?

Comment 14

[Ioana (away)]

Attachment: Screenshot from 2014-02-05 16:05:50.png

Re-did both runs (js...686-buggy is the 10/31 shell, js..686 is the 02/04 beta shell) and took a screenshot to make this go easier.

Comment 15

[Ioana (away)]

Gave it a try with the 02/05 Nightly shell and I get the same:
Assertion failure: isObject(), at ../../../../dist/include/js/Value.h:1124
Segmentation fault (core dumped)

Ran it with --fuzzing-safe here too.

Comment 16

[Ioana (away)]

FWIW, if I run this test without the flag, I get the exact same thing (no crash, just the assertion) on all versions.

Comment 17

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Ioana, the flag should be inserted after the name of the js binary but before the name of testcase:

$ ./js-dbg-64-ts-er-darwin-262e73a6b7cd 926431.js
926431.js:warning: Successfully compiled asm.js code (total compilation time 13ms; not stored in cache)
Assertion failure: isObject(), at ../../../dist/include/js/Value.h:1124
Segmentation fault: 11

$ ./js-dbg-64-ts-er-darwin-262e73a6b7cd --fuzzing-safe 926431.js
926431.js:warning: Successfully compiled asm.js code (total compilation time 0ms; not stored in cache)
926431.js line 8 > eval:1:0 ReferenceError: clone is not defined

Please retry, and the status flags should also be revised as necessary.

Comment 18

[Ioana (away)]

With the status flag positioned as you specified I get "../test926431.js:8:0 ReferenceError: clone is not defined", so no more assertion. I tested with the beta and nightly js shells.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Benjamin - what's left to do here?  This looks to have been fixed on FF28 already in comment 8 and Ioana looks to have cleared up the confusion about whether this was still reproducible.

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

There is nothing left to do here.

The test case in comment 0 can not be reproduced in any browser build, as this is prevented upfront. This only was a problem in the shell, which fuzzers use to find bugs. The fix was just to prevent fuzzers to call the guilty function (see also comment 3).