Open Bug 949220 Opened 6 years ago Updated Last month

Object creation should be an atomic operation, such that if it succeeds, and reserved slots are immediately written to, finalizers can depend upon those writes having happened

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

REOPENED

People

(Reporter: decoder, Assigned: Waldo, NeedInfo)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:])

Crash Data

Attachments

(1 file)

The following testcase asserts on mozilla-central revision 3ea3d3baa67b (run with --fuzzing-safe):


oomAfterAllocations(16);
Intl.DateTimeFormat.prototype.constructor;
Jason is working on this one already :)
Assignee: general → jorendorff
Crash Signature: [@ udat_close_50]
Keywords: crash
QA Contact: general
Whiteboard: [jsbugmon:update]
Given UDATE_FORMAT_SLOT in DateTimeFormat objects is initialized immediately after object construction in every place, I think this is a duplicate of bug 915336 -- GlobalObject::createBlankPrototype can create an object that the GC can observe (and attempt to finalize, before its reserved slots have been initialized to be GC-safe) but still return nullptr.
Jason and Waldo, I know you guys have been working on fixing these OOM crashes [@ u(num|col|dat)_close_50] in several bugs. Is there any progress on this? I'm still seeing pretty much all of them in the fuzzer. That wouldn't be such a big issue (although it's very annoying and costs processing time), but the assertion tied to this (the DOUBLE_IMPL) is quite generic and possibly covers other security bugs.
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jorendorff)
Fine, I'll look.  The way type objects are set is honestly a disaster, so this is going to be a messy patch, and it might take awhile to finish it.
Flags: needinfo?(jwalden+bmo)
Yeah, I only had the obvious (narrow) fix for this. Waldo's way is better.
Flags: needinfo?(jorendorff)
At this point, given a DOM work week next week, dealing with bug 948583 fallout/patch incompleteness before uplift, dealing with bug 961494 before (another) uplift, getting ICU updated before uplift, and probably one or two other things I've forgotten -- not to mention the incomplete patchwork isn't quite on a machine I can use -- this is not going to happen before the next uplift.  Once these other things wrap up, perhaps.  No sooner.
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Error: Failed to compile specified revision 3ea3d3baa67b (maybe try another?)
Whiteboard: [jsbugmon:] → [jsbugmon:update]
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 3d34a3b6443a).
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:bisectfix]
Whiteboard: [jsbugmon:bisectfix] → [jsbugmon:]
JSBugMon: Fix Bisection requested, result:
=== Tinderbox Build Bisection Results by autoBisect ===

The "bad" changeset has the timestamp "20140414165901" and the hash "ba29a8ec2973".
The "good" changeset has the timestamp "20140414172403" and the hash "21aca7217e7a".

Likely fix window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ba29a8ec2973&tochange=21aca7217e7a
Duplicate of this bug: 1100480
:Waldo duped bug 1100480 here, and this seemed like it's going to fall off the radar, so assigning and setting needinfo? to him, as per real-life conversation.
Assignee: jorendorff → jwalden+bmo
Flags: needinfo?(jwalden+bmo)
Attached patch HackaroundSplinter Review
The deal is that when we allocate Intl objects, *internally* we do so in a three-step process:

1) allocate the object
2) allocate a thing for its typeobject
3) init its reserved slot to privatevalue(nullptr)

or something like that, memory hazy.  If we succeed in step 1 but fail in step 2, then the reserved slot is still UndefinedValue() -- but the finalizer will get called when the object's GC'd.  Hack around this stupidity by detecting the still-undefined case.
Attachment #8554921 - Flags: review?(efaustbmo)
Duplicate of this bug: 1126072
Comment on attachment 8554921 [details] [diff] [review]
Hackaround

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

OK, this is like the third dumbest thing. r=me
Attachment #8554921 - Flags: review?(efaustbmo) → review+
Landed a hackaround for now.  The underlying issue still remains, just that these three finalizers defend against it happening.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c33995cc288

Hopefully I can get back to fixing the object-creation process to not have this visibly-two-step process in case of OOM after the first step at some point.  Leaving open until that happens and we can remove the extra defensive code this hack added.
Flags: needinfo?(jwalden+bmo)
Keywords: leave-open
OS: Linux → All
Hardware: x86_64 → All
Summary: Assertion failure: JSVAL_IS_DOUBLE_IMPL(data), at dist/include/js/Value.h:1170 or Crash [@ udat_close_50] with OOM → Object creation should be an atomic operation, such that if it succeeds, and reserved slots are immediately written to, finalizers can depend upon those writes having happened
Closing because no crash reported since 12 weeks.
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → WONTFIX
Closing because no crash reported since 12 weeks.
This issue still exists, and "no crash" is not an indicator of its having been fixed.  We're still just hacking around this everywhere.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Flags: needinfo?(sdetar)

Jeff, should still leave this bug open (leave-open keyword)?

Flags: needinfo?(sdetar) → needinfo?(jwalden)

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Flags: needinfo?(sdetar)

Jeff could you help determine if the leave-open keyword should be removed from this bug?

Flags: needinfo?(sdetar)
You need to log in before you can comment on or make changes to this bug.