Closed Bug 1451625 Opened 2 years ago Closed 2 years ago

Crash in mozilla::dom::XULDocument::LoadOverlayInternal

Categories

(Core :: XUL, defect, P1, critical)

Unspecified
Windows 10
defect

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)
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.
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.
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?
(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.
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.
FWIW, this is currently the #8 main process crash on Nightly.
Assignee: nobody → bdahl
Flags: needinfo?(bdahl)
Can we still crash if we're in automation?
(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)
(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 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+
(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)
Priority: -- → P1
(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)
Attachment #8967566 - Attachment is obsolete: true
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 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+
(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... :-)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cc6b8e998c4d
Error on overlay load instead of crash. r=mystor
https://hg.mozilla.org/mozilla-central/rev/cc6b8e998c4d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.