Closed
Bug 1160887
Opened 9 years ago
Closed 9 years ago
Fix various unboxed object bugs
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
Attachments
(3 files)
17.84 KB,
patch
|
jandem
:
review+
nbp
:
review-
|
Details | Diff | Splinter Review |
3.08 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
2.56 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
The attached patch fixes several bugs with unboxed objects which showed up when testing on Try.
Attachment #8600706 -
Flags: review?(jdemooij)
Updated•9 years ago
|
Attachment #8600706 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 1•9 years ago
|
||
Fix a couple more bugs.
Assignee: nobody → bhackett1024
Attachment #8601859 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•9 years ago
|
||
Inline caches in the JIT can write to the expando object attached to an unboxed plain object instead of the unboxed object itself. This is a problem if a post barrier was only triggered on the unboxed object, as if both the unboxed object and its expando are tenured then the tenured->nursery edge won't be traced. This patch fixes this by having store buffer entries on an unboxed object implicitly include the expando, if any, as fixing this in the JIT is pretty hard. I also removed a call to ArgumentsObject::trace that seems redundant given the traceChildren call.
Attachment #8601862 -
Flags: review?(terrence)
Comment 3•9 years ago
|
||
Comment on attachment 8601859 [details] [diff] [review] more bugs Review of attachment 8601859 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/ObjectGroup.cpp @@ +501,5 @@ > clasp = &PlainObject::class_; > } > > + if (proto.isObject() && !proto.toObject()->isDelegate() && !proto.toObject()->setDelegate(cx)) > + return nullptr; JSObject::setFlags does nothing if the object already has all flags, so the !isDelegate check could be removed here. Seems fine either way though.
Attachment #8601859 -
Flags: review?(jdemooij) → review+
Comment 4•9 years ago
|
||
Comment on attachment 8601862 [details] [diff] [review] store buffer fix Review of attachment 8601862 [details] [diff] [review]: ----------------------------------------------------------------- Works for me. ::: js/src/gc/StoreBuffer.cpp @@ -53,5 @@ > JSGCTraceKind kind = GetGCThingTraceKind(edge); > if (kind <= JSTRACE_OBJECT) { > JSObject* object = static_cast<JSObject*>(edge); > - if (object->is<ArgumentsObject>()) > - ArgumentsObject::trace(trc, object); Thanks! I have been meaning to research why I added this and figure out if it was still needed; I guess if it passes Try, then we can just remove it.
Attachment #8601862 -
Flags: review?(terrence) → review+
Comment 5•9 years ago
|
||
Also, I just checked in a patch (bug 1161726) moving this function to Marking.cpp; sorry about the conflict!
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/408e353d81a3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 8•9 years ago
|
||
Comment on attachment 8600706 [details] [diff] [review] patch Review of attachment 8600706 [details] [diff] [review]: ----------------------------------------------------------------- I would prefer us to we fix the code than disabling test case which are verifying that bugs are fixed, such as Bug 1014649.
Attachment #8600706 -
Flags: review-
You need to log in
before you can comment on or make changes to this bug.
Description
•