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

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM
--
critical
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

({crash})

unspecified
mozilla54
x86
Windows 7
crash
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 wontfix, firefox52 fixed, firefox53 fixed, firefox54 fixed)

Details

(crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

a year ago
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
(Assignee)

Comment 1

a year ago
Created attachment 8831328 [details] [diff] [review]
Don't crash in MessagePort is cycle collected while a message event is pending. r=baku

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"
(Assignee)

Comment 4

a year ago
Created attachment 8831541 [details] [diff] [review]
Make PostMessageRunnable handle the port being cycle collected. r=baku

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95634cd2c25dd41bad9c5629627f52373c0734d4
Attachment #8831328 - Attachment is obsolete: true
Attachment #8831541 - Flags: review?(amarchesini)
(Assignee)

Comment 5

a year ago
Created attachment 8831543 [details] [diff] [review]
Make PostMessageRunnable handle the port being cycle collected. r=baku

Fix opt build.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ec98a6e354410dd146dc06333a9759010fd2021
Attachment #8831541 - Attachment is obsolete: true
Attachment #8831541 - Flags: review?(amarchesini)
Attachment #8831543 - Flags: review?(amarchesini)
Attachment #8831543 - Flags: review?(amarchesini) → review+

Comment 6

a year ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0182967f7703
Make PostMessageRunnable handle the port being cycle collected. r=baku
(Assignee)

Comment 7

a year ago
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?

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0182967f7703
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
status-firefox52: --- → affected
status-firefox53: --- → affected
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+
(Assignee)

Comment 10

a year ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/d1569ed822af

https://hg.mozilla.org/releases/mozilla-beta/rev/339082e4a33a
status-firefox52: affected → fixed
status-firefox53: affected → fixed
Flags: qe-verify-
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
status-firefox51: --- → affected

Comment 12

11 months ago
No plan to have dot release for 51. Mark 51 won't fix.
status-firefox51: affected → wontfix
You need to log in before you can comment on or make changes to this bug.