GC: Fix some shell rooting hazards false positives

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

Trunk
mozilla23
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

This is a patch to address some of the false positives in the shell rooting analysis:

Fix false positives in js::CloneFunctionAtCallsite by not leaving unrooted CallsiteCloneKey on the stack over potential GC.  This does keep pointers to GC things cast to void * so we can tell if they moved, and avoids relooking up the hashtable pointer in the case where they have not.  Also, fixes the condition to test for this, and updates the add pointer.

Replace PropDesc::AutoRooter with AutoPropDescRooter, which contains the rooted PropDesc.

Replace RegExpStatics::AutoRooter with AutoRegExpStaticsBuffer, which contains a rooted RegExpStatics to be used as a buffer.

In EmptyShape::getInitialShape, don't keep InitialShapeEntry::Lookup live over potential GC.
Attachment #744169 - Flags: review?(terrence)
Comment on attachment 744169 [details] [diff] [review]
Proposed changes

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

r=me with nits fixed.

::: js/src/jscntxt.cpp
@@ +256,5 @@
>          return NULL;
>  
> +    uint32_t offset = pc - script->code;
> +    void* originalScript = script;
> +    void* originalFun = fun;

Aren't these going to need a SkipRoot to keep the analysis from poisoning them?

::: js/src/vm/ObjectImpl.h
@@ +297,5 @@
> +{
> +  public:
> +    explicit AutoPropDescRooter(JSContext *cx
> +                                MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
> +        : CustomAutoRooter(cx), skip(cx, &propDesc)

2 space indent for :.

@@ +302,3 @@
>          {
>              MOZ_GUARD_OBJECT_NOTIFIER_INIT;
>          }

I think this whole block needs to be dedented one level now.

@@ +336,5 @@
> +    bool writable() const { return propDesc.writable(); }
> +
> +    HandleValue value() const { return propDesc.value(); }
> +    JSObject * getterObject() const { return propDesc.getterObject(); }
> +    JSObject * setterObject() const { return propDesc.setterObject(); }

Extra spaces after *s.

@@ +358,3 @@
>  };
>  
> +

Extra newline.

::: js/src/vm/RegExpStatics-inl.h
@@ +158,3 @@
>          {
>              MOZ_GUARD_OBJECT_NOTIFIER_INIT;
>          }

I think this block needs dedenting 1 level.

::: js/src/vm/Shape.cpp
@@ -1244,5 @@
>          && lookup.parent == shape->getObjectParent()
>          && lookup.nfixed == shape->numFixedSlots()
>          && lookup.baseFlags == shape->getObjectFlags();
>  }
> -

I think we actually want a newline here.

@@ +1283,2 @@
>          return NULL;
> +        }

The spacing of these {} look off: I think you want a single 4 space indent for each of them.
Attachment #744169 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #1)

Ugh.  Thanks, fixed.
https://hg.mozilla.org/mozilla-central/rev/85430cf0c1a7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.