Closed Bug 1511130 Opened 1 year ago Closed 1 year ago

crash near null in [@ mozilla::dom::ShadowRoot_Binding::get_host]

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 --- fixed
firefox66 --- fixed

People

(Reporter: tsmith, Assigned: timdream)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Attachments

(4 files)

Attached file testcase.html
==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>)
Flags: in-testsuite?
Attached file prefs.js
Interesting. I wonder if we should just make https://searchfox.org/mozilla-central/source/dom/webidl/ShadowRoot.webidl#24 nullable for now.
Flags: needinfo?(emilio)
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.
Blocks: 1508000
(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.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
(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 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.)
Depends on: 1510848
Is this caused by the fact that we finally are running some destructors in tests?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8029a83f46a87fc1edc4eb9192f40c6e48b57a42
No longer depends on: 1510848
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
Flags: needinfo?(emilio)
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
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Is this something we should consider for backport consideration or can it ride the trains?
Flags: needinfo?(timdream)
Flags: in-testsuite?
Flags: in-testsuite+
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:
Flags: needinfo?(timdream)
Attachment #9029124 - Flags: approval-mozilla-beta?
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:
Attachment #9029125 - Flags: approval-mozilla-beta?
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.
Attachment #9029124 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9029125 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.