Closed Bug 1288581 Opened 3 years ago Closed 3 years ago

Trace the expando object of shadowing DOM proxies from the proxy itself, not from CC code

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files)

Right now we can end up (and typically do!) in a situation where the reflector (the proxy) is black, but the expando object is gray, because it's only kept alive by the CC.  This makes it easy to end up working with a gray object when we don't really want to be; see the try failures for the patch in bug 1283634.

Seems like it should be pretty simple to just trace the expando from the proxy instead.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8773575 [details] [diff] [review]
part 1.  Start tracing the expando object, if any, inside the ExpandoAndGeneration of a shadowing DOM proxy from that proxy's trace hook

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

::: dom/bindings/DOMJSProxyHandler.h
@@ +32,5 @@
>     *
>     * If it is, the proxy is initialized with a PrivateValue, which contains a
>     * pointer to a js::ExpandoAndGeneration object; this contains a pointer to
> +   * the actual expando object as well as the "generation" of the object.  The
> +   * proxy handler will handle tracing the expando object stored in the

Nit: "handle tracing" -> "trace"?
Comment on attachment 8773576 [details] [diff] [review]
part 2.  Stop messing with the ExpandoAndGeneration's expando in CC code, since it's now traced by the reflector proxy

> 
>   tmp->mIdentifierMap.Clear();
>-  tmp->mExpandoAndGeneration.Unlink();
>+
>+  // Rev the generation on our mExpandoAndGeneration, since we just cleared the
>+  // identifier map.
>+  ++tmp->mExpandoAndGeneration.generation;
As discussed, rename Unlink() to OwnerUnlinked() and 
call it rather than modifying the generation manually.


> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(HTMLFormElement,
>                                                 nsGenericHTMLElement)
>   tmp->Clear();
>-  tmp->mExpandoAndGeneration.Unlink();
>+  ++tmp->mExpandoAndGeneration.generation;
ditto

>+++ b/dom/html/nsDOMStringMap.cpp
>@@ -27,25 +27,20 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(ns
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
>   // Check that mElement exists in case the unlink code is run more than once.
>   if (tmp->mElement) {
>     // Call back to element to null out weak reference to this object.
>     tmp->mElement->ClearDataset();
>     tmp->mElement->RemoveMutationObserver(tmp);
>     tmp->mElement = nullptr;
>   }
>-  tmp->mExpandoAndGeneration.Unlink();
>+  ++tmp->mExpandoAndGeneration.generation;
ditto
Attachment #8773576 - Flags: review?(peterv) → review+
Attachment #8773575 - Flags: review?(peterv) → review?(bugs)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/541edb687906
part 1.  Start tracing the expando object, if any, inside the ExpandoAndGeneration of a shadowing DOM proxy from that proxy's trace hook.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab1504ebfbde
part 2.  Stop messing with the ExpandoAndGeneration's expando in CC code, since it's now traced by the reflector proxy.  r=smaug
Attachment #8773575 - Flags: review?(bugs) → review+
Comment on attachment 8773575 [details] [diff] [review]
part 1.  Start tracing the expando object, if any, inside the ExpandoAndGeneration of a shadowing DOM proxy from that proxy's trace hook

># HG changeset patch
># User Boris Zbarsky <bzbarsky@mit.edu>
># Date 1469154494 14400
>#      Thu Jul 21 22:28:14 2016 -0400
># Node ID 1ce2d3d8b1831d84d05d4bbbac39c7a7f003f7a6
># Parent  74d28c3922b2519cccb1f41bc0723c643ba63ba0
>Bug 1288581 part 1.  Start tracing the expando object, if any, inside the ExpandoAndGeneration of a shadowing DOM proxy from that proxy's trace hook.  r=peterv.
>
>diff --git a/dom/bindings/Codegen.py b/dom/bindings/Codegen.py
> 
+// static
>+void
>+ShadowingDOMProxyHandler::trace(JSTracer* trc, JSObject* proxy) const
This isn't static method.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b88f94ed312
followup.  Remove a bogus comment.  DONTBUILD.
> This isn't static method.

Fixed in the comment 7 commit.
Depends on: 1294747
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.