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.

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

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.

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

I don't have a access to bug 1481844.

(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 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.

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

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

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

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

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.

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.

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