1483487 - Improve assertions around DOM proxy expando objects

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

Bug 1481844 raises the possibility that DOMProxyHandler::EnsureExpandoObject can return a dead expando object.  We can add some assertions to check whether this is happening and ensure we're in the correct state here.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1483487-dom-proxy-asserts

Add some asserts to check everything is as we expect.  This is green on try.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9000190 [details] [diff] [review]
bug1483487-dom-proxy-asserts

I'd prefer peterv to take a look at this. But if he is on vacation or something, I could review.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1483487-dom-proxy-asserts-2

Also, we should check that the expando field of ExpandoAndGeneration is not set when we set up the proxy object's private pointer.

Comment 4

[Peter Van der Beken [:peterv]]

I don't have a access to bug 1481844.

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Peter Van der Beken [:peterv] from comment #4)
> I don't have a access to bug 1481844.

I've CCed you.

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 9000190 [details] [diff] [review]
bug1483487-dom-proxy-asserts

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

::: dom/bindings/DOMJSProxyHandler.cpp
@@ +79,5 @@
> +
> +  nsISupports* native = UnwrapDOMObject<nsISupports>(proxy);
> +  nsWrapperCache* cache;
> +  CallQueryInterface(native, &cache);
> +  MOZ_ASSERT(cache->PreservingWrapper());

Hmm, this needs a comment, because I don't think this check holds in general. Unlinking clears the flag signaling that we're preserving the wrapper but doesn't clear the expando object. I think we can do this check in the cases below because nothing should call those functions on a binding for an object that was unlinked.

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Peter Van der Beken [:peterv] from comment #6)
Thanks, comment added.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76659fa6e0e0
Add asertions around creating and retrieving DOM proxy expando objects r=peterv

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2dd17272637
Fix static analysis bustage and add missing comments on a CLOSED TREE r=me

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/76659fa6e0e0
https://hg.mozilla.org/mozilla-central/rev/d2dd17272637

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/a8f720d6e51e

Comment 12

[Ryan VanderMeulen [:RyanVM]]

I had landed this on ESR60 so bug 1481844 would uplift cleanly, but that ended badly. Backed out for assertions and rooting hazards:
https://hg.mozilla.org/releases/mozilla-esr60/rev/999249ba5dac930e6d589471c38459be1f867234

https://treeherder.mozilla.org/logviewer.html#?job_id=203491173&repo=mozilla-esr60
https://treeherder.mozilla.org/logviewer.html#?job_id=203494541&repo=mozilla-esr60

Maybe we don't need this at all and we can just take a different patch in bug 1481844, will defer to Jon on that when he's back from PTO.

Comment 13

[Jon Coppeard (:jonco)]

I tested this patch on esr60.  The rooting hazard needs a suppression from bug 1487167.  There is still an assertion failure though:

Assertion failure: cache->PreservingWrapper(), at /builds/worker/workspace/build/src/dom/bindings/DOMJSProxyHandler.cpp:87
#01: mozilla::dom::CheckExpandoAndGeneration [dom/bindings/DOMJSProxyHandler.cpp:99]
#02: mozilla::dom::DOMProxyHandler::GetExpandoObject [js/public/Value.h:1370]
#03: mozilla::dom::HTMLDocumentBinding::DOMProxyHandler::get [s3:gecko-generated-sources-l1:1497ec9186a52beaab0a9d4396efe3c33dcb511a394fedac9c91541e44fa1e0d867a30ac19f4a6f5e70139b9c9bd3a3eb0fbe79fcad2df32e4a453f067e4e231/dom/bindings/HTMLDocumentBinding.cpp::2096]
#04: js::Proxy::getInternal [js/src/proxy/Proxy.cpp:333]
#05: js::Proxy::get [js/src/proxy/Proxy.cpp:360]
#06: js::GetProperty [js/src/vm/NativeObject.h:1630]

The review in comment 6 does say this this condition doesn't hold in the general case.  But we haven't seen this fail on central and I don't think this path should be happening for an object that has been unlinked.

Peter, do you know what's happening here?  I couldn't see any obvious intervening changes on central that would explain the difference.

Comment 14

[Peter Van der Beken [:peterv]]

I looked around a bit, but don't really understand why we'd be hitting that only on branch.