Closed Bug 823715 Opened 7 years ago Closed 7 years ago

"Assertion failure: JSVAL_IS_DOUBLE_IMPL(data),"

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla20

People

(Reporter: gkw, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [fuzzblocker] [jsbugmon:testComment=2,origRev=868b21bed3eb])

Attachments

(2 files)

Attached file stack
for (var i = 0; i < 9; ++i) {
    var g = this[Object.getOwnPropertyNames(this)[i]];
    for (var j = 0; j < 9; ++j) {
        try {
            g.prototype[Object.getOwnPropertyNames(g.prototype)[j]]
        } catch (e) {}
    }
}

asserts js debug shell on m-c changeset 868b21bed3eb without any CLI arguments at Assertion failure: JSVAL_IS_DOUBLE_IMPL(data),

This testcase requires --enable-more-deterministic but I think I can get a testcase without --enable-more-deterministic.

jsfunfuzz hits this very very early on prior to generation of any random code, so having this bug renders jsfunfuzz almost inoperable.
Whiteboard: [fuzzblocker][jsbugmon:update] → [fuzzblocker] [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
for (var i = 0; i < 99; ++i) {
    var g = this[Object.getOwnPropertyNames(this)[i]];
    for (var j = 0; j < 9; ++j) {
        try {
            g.prototype[Object.getOwnPropertyNames(g.prototype)[j]];
        } catch (e) {}
    }
}

This testcase does not require --enable-more-deterministic - I think the only difference is the first for-loop needing to loop 99 times here instead of only 9 times in comment 0.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   116633:91dae8287643
user:        Jan de Mooij
date:        Thu Dec 20 15:27:54 2012 +0100
summary:     Bug 822385 - Add getter, setter and method with jitinfo to the shell. r=bz

jandem, this is pretty bad, do you mind looking at this asap?
Blocks: 822385
Flags: needinfo?(jdemooij)
Whiteboard: [fuzzblocker] [jsbugmon:] → [fuzzblocker] [jsbugmon:update,testComment=1]
Whiteboard: [fuzzblocker] [jsbugmon:update,testComment=1] → [fuzzblocker] [jsbugmon:update,testComment=1,origRev=868b21bed3eb]
Whiteboard: [fuzzblocker] [jsbugmon:update,testComment=1,origRev=868b21bed3eb] → [fuzzblocker] [jsbugmon:testComment=1,origRev=868b21bed3eb]
JSBugMon: Cannot process bug: Error: Failed to isolate test from comment
Whiteboard: [fuzzblocker] [jsbugmon:testComment=1,origRev=868b21bed3eb] → [fuzzblocker] [jsbugmon:update,testComment=2,origRev=868b21bed3eb]
Oops.  This is definitely a bug in the patch for bug 822385.  This part:

>+    JS::Value val = js::GetReservedSlot(obj, DOM_OBJECT_SLOT);

Is wrong because the shell object setup differs from actual DOM objects in a crucial way.  In particular, the "DOM proto" is set up with the same JSClass as the "DOM objects" in the shell, which is NOT how actual DOM objects work.  But the proto of course does not have a useful private set.  So we pass the class check in dom_generic*, but then try to val.toPrivate() on the reserved slot, and that fails.

We should probably just use ObjectClass for the proto.
Attached patch PatchSplinter Review
I'm not sure how to use the Object proto as prototype and still be able to use InitClass. Furthermore, I'm not sure if the IonMonkey paths still work with that. So this patch just initializes FakeDOMObject.prototype.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attachment #694741 - Flags: review?(bzbarsky)
Flags: needinfo?(jdemooij)
Comment on attachment 694741 [details] [diff] [review]
Patch

The Ion paths should work fine with that, but yeah, you can't JS_InitClass, I guess.

r=me; at some point if it becomes needed we can make this look more like actual DOM objects.
Attachment #694741 - Flags: review?(bzbarsky) → review+
Whiteboard: [fuzzblocker] [jsbugmon:update,testComment=2,origRev=868b21bed3eb] → [fuzzblocker] [jsbugmon:update,testComment=2,origRev=868b21bed3eb,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision ea373e534245).
https://hg.mozilla.org/mozilla-central/rev/a431a8e935bf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [fuzzblocker] [jsbugmon:update,testComment=2,origRev=868b21bed3eb,ignore] → [fuzzblocker] [jsbugmon:update,testComment=2,origRev=868b21bed3eb]
Target Milestone: --- → mozilla20
A variant of the test landed -> in-testsuite+ and VERIFIED on behalf of jsbugmon.
Status: RESOLVED → VERIFIED
Flags: in-testsuite? → in-testsuite+
Whiteboard: [fuzzblocker] [jsbugmon:update,testComment=2,origRev=868b21bed3eb] → [fuzzblocker] [jsbugmon:testComment=2,origRev=868b21bed3eb]
You need to log in before you can comment on or make changes to this bug.