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

RESOLVED FIXED in Firefox 67

Status

()

defect
P2
critical
RESOLVED FIXED
5 months ago
3 months ago

People

(Reporter: philipp, Assigned: Nika)

Tracking

({crash, regression})

67 Branch
mozilla68
All
Windows
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66 unaffected, firefox67+ fixed, firefox68 fixed)

Details

(crash signature)

Attachments

(2 attachments)

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.

Flags: needinfo?(nika)
Priority: -- → P2

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

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.

Flags: needinfo?(nika)
Assignee: nobody → nika
Pushed by nlayzell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1837a4550e10
Don't send WindowGlobalChild constructors when parent BrowsingContext has already been closed, r=farre
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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

Flags: needinfo?(nika)

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

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
Flags: needinfo?(nika)
Attachment #9057984 - Flags: approval-mozilla-beta?

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!

Attachment #9057984 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Flags: needinfo?(nika)
Flags: qe-verify-

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Nika, any idea?

Flags: needinfo?(nika)

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

Flags: needinfo?(afarre)

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.

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.

Duplicate of this bug: 1545996
Flags: needinfo?(afarre)

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

Pushed by nlayzell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7376b80d14d7
Ensure origin process is subscribed when attaching remote BC, r=farre

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
Flags: needinfo?(nika)
Attachment #9062009 - Flags: approval-mozilla-beta?

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.

Attachment #9062009 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Crash Signature: [@ mozilla::dom::WindowGlobalParent::WindowGlobalParent] → [@ mozilla::dom::WindowGlobalParent::WindowGlobalParent] [@ mozilla::ipc::FatalError(char const*, bool)]
Status: REOPENED → RESOLVED
Closed: 4 months ago4 months ago
Resolution: --- → FIXED
Crash Signature: [@ mozilla::dom::WindowGlobalParent::WindowGlobalParent] [@ mozilla::ipc::FatalError(char const*, bool)] → [@ mozilla::dom::WindowGlobalParent::WindowGlobalParent] [@ mozilla::ipc::FatalError(char const*, bool)]

(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.

Crash Signature: [@ mozilla::dom::WindowGlobalParent::WindowGlobalParent] [@ mozilla::ipc::FatalError(char const*, bool)] → [@ mozilla::dom::WindowGlobalParent::WindowGlobalParent]
Flags: needinfo?(nika)
Crash Signature: [@ mozilla::dom::WindowGlobalParent::WindowGlobalParent] → [@ mozilla::dom::WindowGlobalParent::WindowGlobalParent] [@ mozilla::ipc::FatalError(char const*, bool)]
You need to log in before you can comment on or make changes to this bug.