Closed Bug 1334677 Opened 3 years ago Closed 3 years ago
Crash in mozilla::dom::Post
Message Runnable::Dispatch Message W
This bug was filed from the Socorro interface and is report bp-b0925d5c-029d-4df9-94ca-cdd042170127. ============================================================= It looks like mPort can be cycle collected while the runnable is in the event queue: https://dxr.mozilla.org/mozilla-central/source/dom/messagechannel/MessagePort.cpp#170
This is my attempt to fix the issue. It makes cycle collection use the runnable's Cancel() method. I then make Cancel() threadsafe with the Run() method. Its not actually clear to me, though, if this runnable executes across threads or not. If not, I think we can simplify to just a nullptr check in the Run() with perhaps some thread assertions. https://treeherder.mozilla.org/#/jobs?repo=try&revision=33fabecb27e4393ab1ce7bfb80ab536fb9f383ac
Attachment #8831328 - Flags: review?(amarchesini)
Comment on attachment 8831328 [details] [diff] [review] Don't crash in MessagePort is cycle collected while a message event is pending. r=baku Review of attachment 8831328 [details] [diff] [review]: ----------------------------------------------------------------- PostMessageRunnable is created and dispatched to the current thread. It's not thread-safe and it doesn't need to be. You are right when you say that we just need to check if mPort is null. With an "important" comment explaining that, if MessagePort is CCed, mPostMessageRunnable->mPort is nullified and, when Run() is executed, mPort is null. Thanks for checking this!
Attachment #8831328 - Flags: review?(amarchesini) → review-
The last sentence was: "Thanks for catching this"
Attachment #8831543 - Flags: review?(amarchesini) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0182967f7703 Make PostMessageRunnable handle the port being cycle collected. r=baku
Comment on attachment 8831543 [details] [diff] [review] Make PostMessageRunnable handle the port being cycle collected. r=baku Approval Request Comment [Feature/Bug causing the regression]: MessageChannel [User impact if declined]: Crashes. We get a handful every day on release channels. [Is this code covered by automated tests?]: We have tests for this feature, but they don't trigger the race condition leading to the crash. [Has the fix been verified in Nightly?]: Its a race condition and difficult to manually verify [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Minimal risk [Why is the change risky/not risky?]: This adds a nullptr check and some thread safety assertions. [String changes made/needed]: None
Comment on attachment 8831543 [details] [diff] [review] Make PostMessageRunnable handle the port being cycle collected. r=baku guard against race condition to fix a crash in beta52 and aurora53
Crash volume for signature 'mozilla::dom::PostMessageRunnable::DispatchMessageW': - nightly (version 54): 1 crash from 2017-01-23. - aurora (version 53): 0 crashes from 2017-01-23. - beta (version 52): 49 crashes from 2017-01-23. - release (version 51): 537 crashes from 2017-01-16. - esr (version 45): 0 crashes from 2016-08-10. Crash volume on the last weeks (Week N is from 02-06 to 02-12): W. N-1 W. N-2 W. N-3 W. N-4 W. N-5 W. N-6 W. N-7 - nightly 1 0 - aurora 0 0 - beta 30 14 - release 339 70 0 - esr 0 0 0 0 0 0 0 Affected platform: Windows Crash rank on the last 7 days: Browser Content Plugin - nightly #1035 - aurora - beta #575 #478 - release #194 #160 - esr
You need to log in before you can comment on or make changes to this bug.