1565489 - Crash in [@ mozilla::dom::WindowGlobalParent::ContentParentId] during PrintPreview

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[[:philipp]]

This bug is for crash report bp-cf6a5a71-b1cc-48b1-9119-33b270190711.

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::WindowGlobalParent::ContentParentId dom/ipc/WindowGlobalParent.cpp:157
1 xul.dll mozilla::dom::WindowGlobalParent::IsProcessRoot dom/ipc/WindowGlobalParent.cpp:175
2 xul.dll static bool mozilla::dom::WindowGlobalParent_Binding::get_isProcessRoot dom/bindings/WindowGlobalActorsBinding.cpp:1059
3 xul.dll mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3077
4 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:540
5 xul.dll js::CallGetter js/src/vm/Interpreter.cpp:735
6 xul.dll js::NativeGetProperty js/src/vm/NativeObject.cpp:2607
7 xul.dll js::GetProperty js/src/vm/Interpreter.cpp:4492
8 xul.dll static bool Interpret js/src/vm/Interpreter.cpp:2775
9 xul.dll js::RunScript js/src/vm/Interpreter.cpp:425

this browser process crash signature is newly showing up in firefox 69 - possibly related to the changes in bug 1554280.

url correlations show that users are commonly in about:printpreview when the crash is taking place...

Comment 1

[Andrew McCreight [:mccr8]]

Any ideas here, Mike? Maybe the browsing context of the print preview window is null? It looks easy enough to add a null check for BrowsingContext to WindowGlobalParent::IsProcessRoot(), but I'm not sure what the right behavior is. Return false, maybe?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

So here's the code in question:

uint64_t WindowGlobalParent::ContentParentId() {
  RefPtr<BrowserParent> browserParent = GetBrowserParent();
  return browserParent ? browserParent->Manager()->ChildID() : 0;
}

So it looks like we're already null-checking the BrowserParent here... so presumably the BrowserParent->Manager() is nullptr, and we're exploding? I guess we should return 0 in that case too... but I think I'm going to redirect to Nika.

Comment 3

[Mayank Bansal]

comments in bug 1566232 suggest the reported had enabled WR, which caused crashes in printpreview.
Disabling WR fixed this for them

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #2)

So here's the code in question:

uint64_t WindowGlobalParent::ContentParentId() {
  RefPtr<BrowserParent> browserParent = GetBrowserParent();
  return browserParent ? browserParent->Manager()->ChildID() : 0;
}

So it looks like we're already null-checking the BrowserParent here... so presumably the BrowserParent->Manager() is nullptr, and we're exploding? I guess we should return 0 in that case too... but I think I'm going to redirect to Nika.

Looking at the crash report, the line number associated is the first one. This leads me to believe, without looking at the minidump, that we're likely crashing due to an error within GetBrowserParent. The crash offset is 0x68 (~13 pointers-ish), meaning it looks like we read an offset from a null pointer, and then tried to read it. I'm guessing that we have a null WindowGlobalParent object here and are trying to access the mInProcess field from it when this crash is happening.

I don't see anywhere before this call where we'd error out earlier if the stored windowGlobalParent field in the JS reflector is nullptr...

No idea how on earth we'd get in that situation though! The only two places I see where we call isProcessRoot from JS are https://searchfox.org/mozilla-central/rev/07f7390618692fa4f2a674a96b9b677df3a13450/toolkit/actors/BrowserElementParent.jsm#266 and https://searchfox.org/mozilla-central/source/toolkit/content/widgets/browser-custom-element.js#2101, both of which just read the currentWindowGlobal property, which is correctly marked as returning an optional value, so a null return value would be handled correctly: https://searchfox.org/mozilla-central/rev/07f7390618692fa4f2a674a96b9b677df3a13450/dom/chrome-webidl/BrowsingContext.webidl#40

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Here's a version of this crash without WebRender turned on: https://crash-stats.mozilla.org/report/index/6f58e207-4f68-418e-a4cb-6fd3d0190714

Comment 7

[Paul Goldberg]

I just installed Firefox 69b6 and it appears the print preview problem has been fixed with WebRender turned on.

Comment 8

[Paul Goldberg]

I guess I reported too soon on this. Even though the troubleshooting information show WebRender turned on I guess it really did not get turned on until I restarted Firefox. WebRender still crashes print preview.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Tracking wr-69 because it seems like there's still something related to WebRender happening here.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Can we add some debugging code that will catch this earlier or at least validate your theory?

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1565489 - Part 1: Avoid null dereference in IsProcessRoot,

The crash appears to be being caused by the GetEmbedderWindowGlobal call
returning a null value. This patch simply avoids the crash in this case, and
returns a reasonable default value of false, as it should never be true for a
BrowsingContext with a parent without Fission enabled.

This patch should be small enough to safely uplift if necessary.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1565489 - Part 2: Set embedder for BrowsingContext upon frame creation,

This should help ensure that the embedder is always set soon enough that we
don't run into the situation which caused the null deref fixed by part 1.

This patch also adds an assertion earlier in WindowGlobalParent's lifecycle to
ensure that the condition is satisfied. This patch likely should not be uplifted.

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9079461 [details]
Bug 1565489 - Part 1: Avoid null dereference in IsProcessRoot,

Beta/Release Uplift Approval Request

• User impact if declined: Crashes in certain documents due to a null-pointer deref.
• Is this code covered by automated tests?: No
• 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): Adds a simple null pointer check before the invalid code, returning a safe default value.

Only Part 1 is needed to fix the crash, and Part 2 need not be uplifted.

(requesting uplift before r+ because the patch is trivial)

• String changes made/needed: none

Comment 14

[Paul Goldberg]

I just tried print preview again. If I first go the print preview of a email page I get no crashes. If I start Firefox and take a print preview of my Xfinty home page it always crashes. https://my.xfinity.com/?cid=cust

bp-14d5caac-6fa7-4c57-a3d3-dffd20190721 7/21/2019, 12:11 AM
View
bp-d58e9b48-263f-4385-9a8d-c38c10190721 7/21/2019, 12:08 AM
View
bp-97ddf990-bdbf-46c1-9cd5-8771b0190721 7/21/2019, 12:07 AM
View
bp-08044f61-e382-420d-a1cc-6219a0190721 7/21/2019, 12:12 AM

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9079461 [details]
Bug 1565489 - Part 1: Avoid null dereference in IsProcessRoot,

Adds a null check to avoid a crash. Approved for 69.0b7.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17267dd9b228
Part 1: Avoid null dereference in IsProcessRoot, r=farre

Comment 17

[Razvan Maries]

https://hg.mozilla.org/releases/mozilla-beta/rev/9843506318f9

Comment 18

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/17267dd9b228

Comment 19

[Phabricator Automation]

Comment on attachment 9079462 [details]
Bug 1565489 - Part 2: Set embedder for BrowsingContext upon frame creation,

Revision D38723 was moved to bug 1568055. Setting attachment 9079462 [details] to obsolete.

Comment 20

[Daniel Cicas [:dcicas], Release QA]

Hello,

I'm trying to reproduce this issue on Fx 69.0b6, but I cant seem to manage it. Are there maybe any steps that I missed or something? I have Webrender on with Nvidia GeForce 1050 GTX.

Comment 21

[Nika Layzell [:nika] (ni? for response)]

(In reply to Daniel Cicas [:dcicas], Release QA from comment #20)

Hello,

I'm trying to reproduce this issue on Fx 69.0b6, but I cant seem to manage it. Are there maybe any steps that I missed or something? I have Webrender on with Nvidia GeForce 1050 GTX.

I was never able to reproduce this issue, as it seems very unreliable to reproduce. Only certain individuals on certain pages seem to be able to reproduce the error reliably.