Closed Bug 1334677 Opened 3 years ago Closed 3 years ago

Crash in mozilla::dom::PostMessageRunnable::DispatchMessageW

Categories

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

x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 2 obsolete files)

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 bkelly@mozilla.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
Attachment #8831543 - Flags: approval-mozilla-beta?
Attachment #8831543 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/0182967f7703
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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
Attachment #8831543 - Flags: approval-mozilla-beta?
Attachment #8831543 - Flags: approval-mozilla-beta+
Attachment #8831543 - Flags: approval-mozilla-aurora?
Attachment #8831543 - Flags: approval-mozilla-aurora+
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
No plan to have dot release for 51. Mark 51 won't fix.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.