Closed Bug 1160887 Opened 5 years ago Closed 5 years ago

Fix various unboxed object bugs


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox40 --- fixed


(Reporter: bhackett, Assigned: bhackett)




(3 files)

Attached patch patchSplinter Review
The attached patch fixes several bugs with unboxed objects which showed up when testing on Try.
Attachment #8600706 - Flags: review?(jdemooij)
Attachment #8600706 - Flags: review?(jdemooij) → review+
Attached patch more bugsSplinter Review
Fix a couple more bugs.
Assignee: nobody → bhackett1024
Attachment #8601859 - Flags: review?(jdemooij)
Attached patch store buffer fixSplinter Review
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 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 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+
Also, I just checked in a patch (bug 1161726) moving this function to Marking.cpp; sorry about the conflict!
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8600706 [details] [diff] [review]

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.