==42608==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000010 (pc 0x7efef7107c60 bp 0x7fff39a55270 sp 0x7fff39a551a0 T0) ==42608==The signal is caused by a READ memory access. ==42608==Hint: address points to the zero page. #0 0x7efef7107c5f in GetWrapperPreserveColor src/obj-firefox/dist/include/nsWrapperCacheInlines.h:16:13 #1 0x7efef7107c5f in nsWrapperCache::GetWrapper() const src/obj-firefox/dist/include/nsWrapperCacheInlines.h:31 #2 0x7efefcb6adf9 in DoGetOrCreateDOMReflector<mozilla::dom::Element, mozilla::dom::binding_detail::eWrapIntoContextCompartment> src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1075:26 #3 0x7efefcb6adf9 in GetOrCreateDOMReflector<mozilla::dom::Element> src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1151 #4 0x7efefcb6adf9 in mozilla::dom::ShadowRoot_Binding::get_host(JSContext*, JS::Handle<JSObject*>, mozilla::dom::ShadowRoot*, JSJitGetterCallArgs) src/obj-firefox/dom/bindings/ShadowRootBinding.cpp:105 #5 0x7efefe594220 in bool mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3193:13 #6 0x7eff070d8ce0 in js::jit::CallNativeGetter(JSContext*, JS::Handle<JSFunction*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) src/js/src/jit/VMFunctions.cpp:1587:10 #7 0x3bb487d8107f (<unknown module>)

Interesting. I wonder if we should just make https://searchfox.org/mozilla-central/source/dom/webidl/ShadowRoot.webidl#24 nullable for now.

So, this is a regression from bug 1508000, because now we actually can run that code. Now there's an issue. In this case (and in bug 1511138), nsContentUtils::DispatchChromeEvent fails, because we're adopting into about:blank, which has no window, and so we return here: https://searchfox.org/mozilla-central/rev/e22c0d152060f4f8d4ca8904094f15f65a1b6f93/dom/base/nsContentUtils.cpp#4590 Then somehow we end up re-initializing the widget again with the unattached ShadowRoot, which causes the havoc.

(In reply to Olli Pettay [:smaug] (high review load) from comment #2) > Interesting. I wonder if we should just make > https://searchfox.org/mozilla-central/source/dom/webidl/ShadowRoot.webidl#24 > nullable for now. I think we can hit this code now without UAWidget stuff because of BlastSubtreeToPieces, so we may need to do this regardless, but we need to figure out something for comment 3.

I might as well address this in bug 1510848 too. Emilio, I am not sure if you would like to deal with comment 4 with other fixes unrelated to UAWidget events. If not I will dup this bug to bug 1510848 once I can verify that the patch there fixes the case here.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #5) > I might as well address this in bug 1510848 too. It turned out they are not related. The UA Widget is being constructed twice because the first constructor hits bug 1506300, and bug 1505957 is not a complete fix because it only cleans up the DOM, not the already added event listeners. I would need to make UAWidgetsChild somehow be able to get a hold of the UA Widget instance even if it has failed to construct.

To my surprise also, Cu.reportError() didn't print anything on the console, otherwise, it would be much more identifiable.

This is on top of bug 1510848 and its current patch has an existing reftest failure. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e05236c891ba5299b0b133f95e4ce2cfdf3959d0

My approach in bug 1505957 turned out to be flawed.

This patch move the actual widget construction to a onsetup method, allow UAWidgetsChild to hold the reference of the widget instance even if the actual setup (happens in the onsetup call) throws. With the reference of the widget kept, UAWidgetsChild will finally able to call its destructor later on. Depends on D13607

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #7) > To my surprise also, Cu.reportError() didn't print anything on the console, > otherwise, it would be much more identifiable. That's weird, it should be sending it out to the console service. FWIW: I've been preferring to use `console.error` for chrome code ever since we started supporting the Console API in all contexts (along with printing to stdout). I'd be curious if there's a case where Cu.reportError is better, and if not we could consider removing it and rewriting callers the Console API.

(I am working on this bug with bug 1510848 on the same work dir so I might as well test them and land them together.)

Assuming bug 1510848 couldn't resolve in time, these are the rebased patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cd4ae1f361b6f8c37fb161095c893155f55a75d

Is this caused by the fact that we finally are running some destructors in tests? https://treeherder.mozilla.org/#/jobs?repo=try&revision=8029a83f46a87fc1edc4eb9192f40c6e48b57a42

There is one a11y assertion failure. Its timing does not tie to a specific test. https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=216642475&repo=try&lineNumber=2950 The assertion was added in bug 1483487. From the backtrace it looks like to be related to the JS Proxy usage in the code (added in bug 1493525). There are issues with the JS Proxy in the first place (see bug 1493525 comment 6) in the first place so let's see if not using it can fix anything. https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fa8b00f644cf6ecfa7f15743b39b97e82c5e257

So it turned out to be not related to JS Proxy because if I comment out that the test still fails. https://treeherder.mozilla.org/#/jobs?repo=try&searchStr=a11y&revision=6fa8b00f644cf6ecfa7f15743b39b97e82c5e257 I've managed to print JS stack and other stuff. The assertion happens when we try to access the reference of |document| object we kept when destructor() is called. https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=216744610&repo=try&lineNumber=3130 It's previously uncaught because we do not correctly call into the destructor() if the page has system principal, and, in this case, we do not really create a sandbox.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #19) > It's previously uncaught because we do not correctly call into the > destructor() if the page has system principal, and, in this case, we do not > really create a sandbox. Given that there isn't any use of UA Widgets elements on system principal pages outside of tests, I decided to unfix this and do so in a follow-up. That should allow us to land this patch first. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c613463fe9887c2305ae6a1e8e7e0ceafe0adf5

Pushed by tchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bb27071244f9 Part I, Backed out changeset faacbb32e16a (Bug 1511130) r=bgrins https://hg.mozilla.org/integration/autoland/rev/cc75d9afa3bb Part II, Allow UAWidgetsChild to destruct widgets even if it throws during construction r=bgrins

https://hg.mozilla.org/mozilla-central/rev/bb27071244f9 https://hg.mozilla.org/mozilla-central/rev/cc75d9afa3bb

Is this something we should consider for backport consideration or can it ride the trains?

Comment on attachment 9029124 [details] Bug 1511130 - Part I, Backed out changeset faacbb32e16a (Bug 1511130) r=bgrins [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: None User impact if declined: Is this code covered by automated tests?: Yes Has the fix been verified in Nightly?: Yes Needs manual test from QE?: Yes If yes, steps to reproduce: List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): String changes made/needed:

Comment on attachment 9029125 [details] Bug 1511130 - Part II, Allow UAWidgetsChild to destruct widgets even if it throws during construction r=bgrins [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: None User impact if declined: None — assertion failure in debug builds Is this code covered by automated tests?: Yes Has the fix been verified in Nightly?: No Needs manual test from QE?: No If yes, steps to reproduce: List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): This patch changes the way we handle JS error in UA Widget and make it more robust. String changes made/needed:

Comment on attachment 9029124 [details] Bug 1511130 - Part I, Backed out changeset faacbb32e16a (Bug 1511130) r=bgrins [Triage Comment] Fixes a crash found by the fuzzers. Approved for 65.0b6.

https://hg.mozilla.org/releases/mozilla-beta/rev/17a1d1be96f4 https://hg.mozilla.org/releases/mozilla-beta/rev/c41c18787e43