Closed Bug 1588356 Opened 4 months ago Closed 3 months ago

Crash in [@ nsINode::WrapObject]

Categories

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

Unspecified
All
defect

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 + fixed
firefox72 --- fixed

People

(Reporter: jseward, Assigned: edgar)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

This bug is for crash report bp-c0a23330-e324-471e-8d1f-1bc5a0191012.

This crash happened 9 times in 4 different instances in the Windows nightly 20191010214019.

Top 10 frames of crashing thread:

0 xul.dll nsINode::WrapObject dom/base/nsINode.cpp:2669
1 xul.dll static bool mozilla::dom::Element_Binding::get_openOrClosedShadowRoot dom/bindings/ElementBinding.cpp:4257
2 xul.dll mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3132
3 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:549
4 xul.dll JS::Call js/src/jsapi.cpp:2722
5 xul.dll xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::DOMXrayTraits>::get js/xpconnect/wrappers/XrayWrapper.cpp:2089
6 xul.dll js::Proxy::get js/src/proxy/Proxy.cpp:353
7 xul.dll js::Proxy::get js/src/proxy/Proxy.cpp:352
8 xul.dll js::Proxy::get js/src/proxy/Proxy.cpp:352
9 xul.dll js::Proxy::get js/src/proxy/Proxy.cpp:352

Flags: needinfo?(bugmail)

Shadow trees is Edgar territory.

Flags: needinfo?(bugmail) → needinfo?(echen)

Looks as if the crashes started in 71 in Build ID 20191009213914. There are some macOS crashes as well. Possible regression range based on build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=be9a6289486a6f366e431782b84a0c0633f8fec2&tochange=e9783a644016aa9b317887076618425586730d73

Bug 1585823 has some ShadowRoot in the checkin description. adding a ni on emilio as well, in case it is related to that checkin

Flags: needinfo?(emilio)
Keywords: regression

So they are all from this assertion:

The spike is related to https://hg.mozilla.org/mozilla-central/rev/68e29d2cd18276860baa297af5b0621013595800, since that makes it go through WrapNode rather than WrapObject and thus run that assertion.

But that assertion should hold anyway, so it's only uncovering a pre-existing bug.

Jason, have you or any other of your fuzzers seen something like this?

Flags: needinfo?(emilio) → needinfo?(jkratzer)

Emilio, no unfortunately not. I'm going to spin up some targeted shadow root fuzz runs and see if I can't shake it out.

Flags: needinfo?(jkratzer)

Look over the crash reports, almost all of the crashes happen on extension, and https://crash-stats.mozilla.org/report/index/a76a44cc-4d33-4962-9b70-527900191021 indicates that the crash happens on Live Start Page extension. And I could also reproduce the crash with Live Start Page extension installed as well, here is the crash report from my side.

STR:

Flags: needinfo?(echen)

Oh, lovely, thanks for figuring out STR! Do you plan to look further into it? Otherwise I can take a look on rr to see what's going on.

Flags: needinfo?(echen)

I am going to track it for 71 as this is going up since mid-october and the volume is already noticeable on dev edition builds, so this is likely to be be significant in beta/release.

Has STR: --- → yes

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

Oh, lovely, thanks for figuring out STR! Do you plan to look further into it? Otherwise I can take a look on rr to see what's going on.

Yes, I plan to take a look into it.

Flags: needinfo?(echen)
Priority: -- → P1
Flags: needinfo?(echen)
Assignee: nobody → echen
OS: Windows 10 → All

What exactly happened is,

If the wrapper is for extension, it could possibly be nuked after extension's
window is destroyed. Then we will end up getting a DeadWrapper for same
extension next time (if the UAWidgetScopeMap is not yet swept).

Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8e130f2d4809
Store the underlying sandbox object instead of a wrapper to UAWidgetScopeMap; r=bholley
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

Please nominate this for Beta approval when you get a chance.

Flags: needinfo?(echen)
Flags: in-testsuite+

Comment on attachment 9104168 [details]
Bug 1588356 - Store the underlying sandbox object instead of a wrapper to UAWidgetScopeMap;

Beta/Release Uplift Approval Request

  • User impact if declined: Webextension page might crash if it uses ShadowDOM
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • 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): The change here is straightforward and makes more sense. And it has been verified in Nightly.
  • String changes made/needed: None
Flags: needinfo?(echen)
Attachment #9104168 - Flags: approval-mozilla-beta?

Comment on attachment 9104168 [details]
Bug 1588356 - Store the underlying sandbox object instead of a wrapper to UAWidgetScopeMap;

Crash fix, noticeable in beta, uplift approved for 71 beta 6, thanks.

Attachment #9104168 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.