867639 - GC: Fix some shell rooting hazards false positives

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: Proposed changes

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.

Comment 1

[Terrence Cole [: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.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #1)

Ugh.  Thanks, fixed.

Comment 3

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85430cf0c1a7

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85430cf0c1a7