1539959 - Crash in [@ mozilla::dom::WindowGlobalParent::WindowGlobalParent]

Status: RESOLVED FIXED

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

Description

[[:philipp]]

This bug is for crash report bp-5ef9bc49-7f8e-49d4-9e83-6df930190324.

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::WindowGlobalParent::WindowGlobalParent dom/ipc/WindowGlobalParent.cpp:41
1 xul.dll mozilla::dom::TabParent::AllocPWindowGlobalParent dom/ipc/TabParent.cpp:1013
2 xul.dll mozilla::dom::PBrowserParent::OnMessageReceived ipc/ipdl/PBrowserParent.cpp:2214
3 xul.dll void mozilla::ipc::MessageChannel::DispatchMessage ipc/glue/MessageChannel.cpp:2078
4 xul.dll nsresult mozilla::ipc::MessageChannel::MessageTask::Run ipc/glue/MessageChannel.cpp:1968
5 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1179
6 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:482
7 xul.dll void mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:110
8 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308
9 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:290

crash reports with this signature and MOZ_RELEASE_ASSERT(aInit.browsingContext()) (Must be made in BrowsingContext) are newly appearing during the 67.0b cycle.

Comment 1

[Pascal Chevrel:pascalc (PTO until April 26)]

Nika, do you have any insight as to what could cause these new crashes in beta? Thanks

Comment 2

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

Attachment: Bug 1539959 - Don't send WindowGlobalChild constructors when parent BrowsingContext has already been closed,

Comment 3

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

I have a theory that this is caused by creating a WindowGlobalChild within a closed BrowsingContext, and this patch should hopefully help deal with that.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1837a4550e10
Don't send WindowGlobalChild constructors when parent BrowsingContext has already been closed, r=farre

Comment 5

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/1837a4550e10

Comment 6

[Pascal Chevrel:pascalc (PTO until April 26)]

Nika, could you request an uplift to beta please? Thanks

Comment 7

[Neha Kochar [:neha]]

[Tracking Requested - why for this release]:
Crashing in beta, needs an uplift to 67 beta.

Comment 8

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

Comment on attachment 9057984 [details]
Bug 1539959 - Don't send WindowGlobalChild constructors when parent BrowsingContext has already been closed,

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: Crashes in beta
• Is this code covered by automated tests?: No
• 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): Only prevents sending constructor for actor when it would cause crash.
• String changes made/needed: None

Comment 9

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9057984 [details]
Bug 1539959 - Don't send WindowGlobalChild constructors when parent BrowsingContext has already been closed,

Crash fix for a beta only crash, uplift approved for 67 beta 12, thanks!

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

Got a conflict when trying to graft this to beta:
grafting 539136:1837a4550e10 "Bug 1539959 - Don't send WindowGlobalChild constructors when parent BrowsingContext has already been closed, r=farre"
merging dom/ipc/WindowGlobalChild.cpp

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/efb56840fba5

Comment 12

[[:philipp]]

the crash volume didn't noticeably go down on beta after the fix landed...

Comment 13

[Pascal Chevrel:pascalc (PTO until April 26)]

Nika, any idea?

Comment 14

[Neha Kochar [:neha]]

Andreas, this is happening because of non-existent browsingContext. Do you have any ideas?

Comment 15

[Marcia Knous [:marcia]]

Neha asked me to look at the crash correlations in Socorro.

Many of the URLs appear to be involved with printing or print preview. It appears people are generating invoices or pdfs based on the URLs. Many of the sites appear to be behind logins.

Almost all of the crashes are Windows, and primarily Win 7. There are a total of 3 Mac crashes.

• Users visiting https://vsagent-admin.bet9ja.com/ are crashing quite a bit. The site is behind a login.
• https://secure.ssa.gov
• http://indiapostgdsonline.in
• http://trackoncourier.com/

Comment 16

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

Attachment: Bug 1539959 - Ensure origin process is subscribed when attaching remote BC,

We were incorrectly not performing an EnsureSubscribed for the origin
process which a BrowsingContext came from. Normally this would mean that
the BrowsingContext could die in the parent process before the
WindowGlobalParent listener arrived, but that didn't generally happen
due to the low likelihood of a CC occuring between the two messages.

It's likely that this crash was caused by people on lower-end hardware
triggering a print. This would be a longer-running operation in the
content process between the first and second message than usual. On
systems with memory pressure, there would be a higher chance of a CC
occuring between the messages, which would then cause this crash.

This patch correctly connects the origin ContentParent to the
BrowsingContextGroup, which will prevent the CC from destroying our
objects.

In the future, it may be desirable to ensure that this doesn't happen
more reliably by using a ContentParent-local table for looking up
BrowsingContext IDs sent over IPC. This would prevent our current
dependency on the weak pointer behaviour of the current global ID cache.

Unfortunately, this patch adds no tests, and I'm not aware of a good way
to test this edge case to confirm it has been fixed. I believe that this
patch should fix the issue I mention, however.

Comment 18

[Pascal Chevrel:pascalc (PTO until April 26)]

Nika, this patch is r+, could you land it and request an uplift please? Thanks

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7376b80d14d7
Ensure origin process is subscribed when attaching remote BC, r=farre

Comment 20

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

Comment on attachment 9062009 [details]
Bug 1539959 - Ensure origin process is subscribed when attaching remote BC,

Beta/Release Uplift Approval Request

• User impact if declined: Potential crashes
• 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): Performs a small fix which is unlikely to cause issues, ideally preventing the issue at hand.

As this has not been verified to fix the crash unconditionally, there is a chance that the crash will continue after this uplift.

• String changes made/needed: None

Comment 21

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9062009 [details]
Bug 1539959 - Ensure origin process is subscribed when attaching remote BC,

Low risk and fixes a medium size crash on beta, approved for 67 beta 16, thanks.

Comment 22

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e9cc7e9d9c73

Comment 23

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/7376b80d14d7

Comment 24

[Razvan Maries]

New occurrence of bug 1544575 after fix: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=244849357&repo=autoland&lineNumber=3732

Comment 25

[Raul Gurzau (:RaulG)]

New failure here: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=245179870&repo=mozilla-inbound&lineNumber=3825

Comment 26

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

(In reply to Raul Gurzau (:RaulGurzau) from comment #25)

New failure here: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=245179870&repo=mozilla-inbound&lineNumber=3825

This is a generic IPC error. The name ipc::FatalError is extremely generic, and can't be all ascribed to this one bug.

Please do not try to attach all ipc::FatalError crashes to a single bug.

Comment 27

[Intermittent Failures Robot]

7 failures in 4705 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-inbound: 2
• autoland: 4
• mozilla-central: 1

Platform breakdown:

• linux64-shippable: 4
• linux32-shippable: 3

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1539959&startday=2019-05-06&endday=2019-05-12&tree=all