Closed
Bug 1451625
Opened 6 years ago
Closed 6 years ago
Crash in mozilla::dom::XULDocument::LoadOverlayInternal
Categories
(Core :: XUL, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | --- | unaffected |
firefox61 | + | fixed |
People
(Reporter: calixte, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
(Keywords: crash, regression)
Crash Data
Attachments
(1 file, 1 obsolete file)
This bug was filed from the Socorro interface and is report bp-71b97d12-f53d-4d26-8f5a-906ab0180405. ============================================================= Top 10 frames of crashing thread: 0 xul.dll mozilla::dom::XULDocument::LoadOverlayInternal dom/xul/XULDocument.cpp:2274 1 xul.dll mozilla::dom::XULDocument::LoadOverlay dom/xul/XULDocument.cpp:2244 2 xul.dll static bool mozilla::dom::XULDocumentBinding::loadOverlay dom/bindings/XULDocumentBinding.cpp:723 3 xul.dll mozilla::dom::GenericBindingMethod dom/bindings/BindingUtils.cpp:3032 4 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:467 5 xul.dll static bool Interpret js/src/vm/Interpreter.cpp:3084 6 xul.dll js::RunScript js/src/vm/Interpreter.cpp:417 7 xul.dll js::ExecuteKernel js/src/vm/Interpreter.cpp:700 8 xul.dll js::Execute js/src/vm/Interpreter.cpp:732 9 xul.dll JS::CloneAndExecuteScript js/src/jsapi.cpp:4765 ============================================================= There are 10 crashes (from 3 installations) in nightly 61 with buildid 20180404224504. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1448162. [1] https://hg.mozilla.org/mozilla-central/rev?node=61c57ad4f4f4
Flags: needinfo?(bdahl)
Assignee | ||
Comment 1•6 years ago
|
||
I mean, this is a debug build with a bunch of add-ons that look to me like they'd be legacy add-ons, so presumably one of them is loading an overlay. I think we should just resolve wontfix.
Assignee | ||
Comment 2•6 years ago
|
||
Err, sorry, I was confused with MOZ_ASSERT, I guess MOZ_CRASH also crashes opt builds. Either way, I don't think we should do anything.
Comment 3•6 years ago
|
||
FWIW MOZ_ASSERT is enough to prevent overlays from landing in m-c. But this is just a temporary state until we remove the platform support entirely. What will happen to extensions with overlays once the platform support is fully removed - will that cause a crash or will it become an ignored PI?
Assignee | ||
Comment 4•6 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #3) > FWIW MOZ_ASSERT is enough to prevent overlays from landing in m-c. But this > is just a temporary state until we remove the platform support entirely. > What will happen to extensions with overlays once the platform support is > fully removed - will that cause a crash or will it become an ignored PI? I would expect the latter, though I think realistically these crashes are probably because of add-on chrome.manifest entries that would either cause the manifest to fail to load or be ignored, or explicit bootstrap or other JS .loadOverlay() calls that will throw (method won't exist). I'm pretty OK with any of those outcomes.
Assignee | ||
Comment 5•6 years ago
|
||
(in particular, in the crash in comment #0 has a stack that implies we're loading a script that then calls document.loadOverlay(), as do most of the reports I've seen at https://crash-stats.mozilla.com/signature/?product=Firefox&signature=mozilla%3A%3Adom%3A%3AXULDocument%3A%3ALoadOverlayInternal&date=%3E%3D2018-03-29T16%3A34%3A55.000Z&date=%3C2018-04-05T16%3A34%3A55.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1#summary )
Comment 6•6 years ago
|
||
Doesn't sound like we need to be too anxious about this, though it would be nice if we could handle the situation a bit more gracefully than crashing still in the 61 timeframe.
tracking-firefox61:
--- → +
Comment 7•6 years ago
|
||
FWIW, this is currently the #8 main process crash on Nightly.
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Assignee: nobody → bdahl
Flags: needinfo?(bdahl)
Assignee | ||
Comment 9•6 years ago
|
||
Can we still crash if we're in automation?
Comment 10•6 years ago
|
||
(In reply to :Gijs (he/him) from comment #9) > Can we still crash if we're in automation? Is there an environment variable or something I can use?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 11•6 years ago
|
||
(In reply to Brendan Dahl [:bdahl] (Away until Apr 21st) from comment #10) > (In reply to :Gijs (he/him) from comment #9) > > Can we still crash if we're in automation? > > Is there an environment variable or something I can use? You can use xpc::IsInAutomation() from C++. Cu.isInAutomation from JS.
Flags: needinfo?(gijskruitbosch+bugs)
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8967566 [details] Bug 1451625 - Error on overlay load instead of crash. https://reviewboard.mozilla.org/r/236242/#review242800 ::: dom/xul/XULDocument.cpp:2266 (Diff revision 1) > > // XUL overlays are in the process of being removed. In a Firefox build, > // loading an overlay will cause a crash as they are no longer allowed. > // However, overlays are allowed in other applications (e.g. Thunderbird) > // while they work on removing them. See bug 1448162. > #ifdef MOZ_CRASH_XUL_OVERLAYS Please change the name of this define, as it's not correct anymore.
Attachment #8967566 -
Flags: review?(nika) → review+
Comment 13•6 years ago
|
||
(In reply to :Gijs (he/him) from comment #11) > (In reply to Brendan Dahl [:bdahl] (Away until Apr 21st) from comment #10) > > (In reply to :Gijs (he/him) from comment #9) > > > Can we still crash if we're in automation? > > > > Is there an environment variable or something I can use? > > You can use xpc::IsInAutomation() from C++. Cu.isInAutomation from JS. Gijs, any chance you would be able to take this over the finish line? Brendan is out for the next week.
Flags: needinfo?(gijskruitbosch+bugs)
Updated•6 years ago
|
Priority: -- → P1
Assignee | ||
Comment 14•6 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #13) > (In reply to :Gijs (he/him) from comment #11) > > (In reply to Brendan Dahl [:bdahl] (Away until Apr 21st) from comment #10) > > > (In reply to :Gijs (he/him) from comment #9) > > > > Can we still crash if we're in automation? > > > > > > Is there an environment variable or something I can use? > > > > You can use xpc::IsInAutomation() from C++. Cu.isInAutomation from JS. > > Gijs, any chance you would be able to take this over the finish line? > Brendan is out for the next week. Sure.
Assignee: bdahl → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8967566 -
Attachment is obsolete: true
Comment 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8968499 [details] Bug 1451625 - Error on overlay load instead of crash. https://reviewboard.mozilla.org/r/237200/#review243078 ::: dom/xul/XULDocument.cpp:2265 (Diff revision 1) > nsresult rv; > > // XUL overlays are in the process of being removed. In a Firefox build, > // loading an overlay will cause a crash as they are no longer allowed. > // However, overlays are allowed in other applications (e.g. Thunderbird) > // while they work on removing them. See bug 1448162. Please update this comment to mention that they only crash in automation, and error in non-automation contexts. ::: dom/xul/XULDocument.cpp:2271 (Diff revision 1) > -#ifdef MOZ_CRASH_XUL_OVERLAYS > +#ifdef MOZ_BREAK_XUL_OVERLAYS > nsCString docSpec; > mCurrentPrototype->GetURI()->GetSpec(docSpec); > nsCString overlaySpec; > aURI->GetSpec(overlaySpec); > + if (xpc::IsInAutomation()) { I think it would be cleaner to run the logic which logs the message to the console etc. unconditionally, and then just wrap the call to MOZ_CRASH in an if (xpc::IsInAutomation()) {} block. It's pretty unfortunate that there are 2 copies of the "Attempt to load overlay %s into %s\n" formatting. ::: dom/xul/XULDocument.cpp:2283 (Diff revision 1) > + overlaySpec.get(), > + docSpec.get()); > + nsCOMPtr<nsIConsoleService> consoleSvc = > + do_GetService("@mozilla.org/consoleservice;1"); > + if (consoleSvc) { > + consoleSvc->LogStringMessage(NS_ConvertASCIItoUTF16(msg).get()); nit: inconsistent indentation here and in the rest of this function. Most of this file is 4-space indented, but the outer if block is 2-space indented, and this if block is 4-space indented again.
Attachment #8968499 -
Flags: review?(nika) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•6 years ago
|
||
mozreview-review |
Comment on attachment 8968499 [details] Bug 1451625 - Error on overlay load instead of crash. https://reviewboard.mozilla.org/r/237200/#review243100 ::: dom/xul/XULDocument.cpp:2278 (Diff revision 2) > - overlaySpec.get(), > + overlaySpec.get(), > - docSpec.get()); > + docSpec.get()); > - MOZ_CRASH("Attempt to load overlay"); > + nsCOMPtr<nsIConsoleService> consoleSvc = > + do_GetService("@mozilla.org/consoleservice;1"); > + if (consoleSvc) { > + consoleSvc->LogStringMessage(NS_ConvertASCIItoUTF16(msg).get()); Please make this 4 space indent for consistency ::: dom/xul/XULDocument.cpp:2282 (Diff revision 2) > + if (consoleSvc) { > + consoleSvc->LogStringMessage(NS_ConvertASCIItoUTF16(msg).get()); > + } > + printf("%s", msg.get()); > + if (xpc::IsInAutomation()) { > + MOZ_CRASH("Attempt to load overlay."); same here.
Attachment #8968499 -
Flags: review?(nika) → review+
Assignee | ||
Comment 20•6 years ago
|
||
(In reply to Nika Layzell [:mystor] from comment #19) > Please make this 4 space indent for consistency Yeah, sorry about that - I caught myself as I was already pushing to mozreview. Fixed in the update that arrived just before the review... :-)
Comment 21•6 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/cc6b8e998c4d Error on overlay load instead of crash. r=mystor
Comment 22•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cc6b8e998c4d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•