Closed Bug 829798 Opened 7 years ago Closed 7 years ago

Crash [@ WeakMap_set_impl] or [@ WeakMap_set] or "Assertion failure: cx->runtime->preserveWrapperCallback,"

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- unaffected
firefox19 --- unaffected
firefox20 --- fixed
firefox21 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: gkw, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:])

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached file stack
(new WeakMap).set(FakeDOMObject.prototype, this)

asserts js debug shell on m-c changeset 44dcffe8792b without any CLI arguments at Assertion failure: cx->runtime->preserveWrapperCallback, and crashes js opt shell at WeakMap_set_impl with WeakMap_set on the stack.

This seems to be a null deref, but setting s-s just-in-case.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   118460:945ed4328628
user:        Andrew McCreight
date:        Fri Nov 09 10:59:02 2012 -0800
summary:     Bug 777385 - Support (some) Paris bindings objects as weak map keys. r=peterv

Setting to track for 20 and 21, though the regressing bug hasn't landed on aurora 20 yet, it will probably be landed soon, and the fix for this bug should also land on that branch.
This is a shell-only null-deref, so I'm unhiding.

We only trigger the callback if we detect a wrapped native, and I was implicitly relying on that to check that we're in the browser, and thus have defined the callback. Apparently the shell has a FakeDOMObject that passes the check for being a new DOM bindings object, so we hit the callback.

The question is, what is the right thing to do here?

On one hand, we could define a fake callback, but what should that even do? On the other hand, we could add a NULL check, but it seems lame to dynamically check a static property. On the third hand, who cares about the speed of a null check.
Assignee: general → continuation
Group: core-security
Shell only, doesn't really need to be tracked.
Gary said that jandem and bz added this fake DOM object to the shell. Do either of you have any thoughts on what would be appropriate here? How "realistic" is FakeDOMObject?
(In reply to Andrew McCreight [:mccr8] from comment #3)
> Gary said that jandem and bz added this fake DOM object to the shell. Do
> either of you have any thoughts on what would be appropriate here? How
> "realistic" is FakeDOMObject?

FakeDOMObject was added in bug 822385.
It's just realistic enough to trigger JIT optimizations so far.  But we can make it more realistic as needed, no?

The real issue here is the lack of a preserveWrapperCallback in the shell, while the weakmap code assumes that such things should exist, right?  We could just set up a no-op callback in the shell if desired.
Still need to add a test.
Attachment #702041 - Attachment is obsolete: true
Attachment #702400 - Flags: review?(wmccloskey)
Attachment #702400 - Flags: review?(wmccloskey) → review+
Keywords: checkin-needed
Flags: in-testsuite+
Crash Signature: [@ WeakMap_set_impl] [@ WeakMap_set] → [@ WeakMap_set_impl] [@ WeakMap_set]
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unknown exception (check manually)
https://hg.mozilla.org/mozilla-central/rev/08d12e9406f3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Crash Signature: [@ WeakMap_set_impl] [@ WeakMap_set] → [@ WeakMap_set_impl] [@ WeakMap_set]
Comment on attachment 702400 [details] [diff] [review]
add preserve wrapper to the shell that always returns true

[Approval Request Comment]
This patch is needed to allow fuzzing with bug 777385. It only affects the JS shell, so it is not part of the build.

Bug caused by (feature/regressing bug #): 777385
User impact if declined: nothing direct, it just gets in the way of fuzzing
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): not part of the build
String or UUID changes made by this patch: none
Attachment #702400 - Flags: approval-mozilla-aurora?
Comment on attachment 702400 [details] [diff] [review]
add preserve wrapper to the shell that always returns true

approved nptob.
Attachment #702400 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.