Crash in UnblockParsingPromiseHandler::MaybeUnblockParser

RESOLVED FIXED in Firefox 55

Status

()

Core
HTML: Parser
P2
critical
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: calixte, Assigned: kmag)

Tracking

(Blocks: 1 bug, {crash, regression})

Trunk
mozilla55
Unspecified
Windows 7
crash, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

(Whiteboard: [clouseau], crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
This bug was filed from the Socorro interface and is 
report bp-003e7596-6361-45c8-bb84-c77522170401.
=============================================================

There are 26 crashes on nightly with buildid 20170401030204. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1350049.

[1] https://hg.mozilla.org/mozilla-central/rev?node=99726da227dd50ef4c79c5767fb9e0fb64136934
Flags: needinfo?(kmaglione+bmo)
Duplicate of this bug: 1352828
This is turning into a bit of a minefield...

In this case, it looks like the cycle collector is unlinking mDocument while mParser is still alive, which breaks the assumption that mDocument will always be non-null if mParser is.
Flags: needinfo?(kmaglione+bmo)
Sorry this has become a minefield (for those playing along at home, I'm assuming Kris is referring to bug 1333990). Will you be able to take this, Kris?
Flags: needinfo?(kmaglione+bmo)
Priority: -- → P2
Oh, sorry, I thought I'd pushed the patch for this on Monday, but it was a travel day, so I guess it got lost on the train wifi.
Flags: needinfo?(kmaglione+bmo)
Assignee: nobody → kmaglione+bmo
Comment hidden (mozreview-request)

Comment 6

a year ago
mozreview-review
Comment on attachment 8855485 [details]
Bug 1352827: Handle potential null reference during cleanup by CC.

https://reviewboard.mozilla.org/r/127316/#review130240

::: dom/base/test/test_blockParsing.html:102
(Diff revision 1)
> +    let blockerPromise = new Promise(() => {});
> +
> +    // Create a finalization witness so we can be sure that the promises
> +    // have been collected before the end of the test.
> +    let destroyedPromise = TestUtils.topicObserved(TOPIC);
> +    let witness = finalizationWitness.make(TOPIC, Math.random());

Why `Math.random()` instead of a deterministic string derived from `url`? Perhaps `url` itself?
(Assignee)

Comment 7

a year ago
mozreview-review-reply
Comment on attachment 8855485 [details]
Bug 1352827: Handle potential null reference during cleanup by CC.

https://reviewboard.mozilla.org/r/127316/#review130240

> Why `Math.random()` instead of a deterministic string derived from `url`? Perhaps `url` itself?

We only ever have one finalization witness at once, so it doesn't really matter what value we use, and I decided there wasn't much point in checking it. But I suppose it would be easy enough to have the promise resolve to the value and then compare it against the URL.

Comment 8

a year ago
mozreview-review-reply
Comment on attachment 8855485 [details]
Bug 1352827: Handle potential null reference during cleanup by CC.

https://reviewboard.mozilla.org/r/127316/#review130240

> We only ever have one finalization witness at once, so it doesn't really matter what value we use, and I decided there wasn't much point in checking it. But I suppose it would be easy enough to have the promise resolve to the value and then compare it against the URL.

Even if you don't compare it, please make it a non-random value if the randomness has no particular purpose.
Comment hidden (mozreview-request)
Putting this on needinfo radar for comment 8. (Not sure if I should change the reviewboard status instead. As a review requestor, it's been annoying to recover when reviewers tried to communicate "action needed" via reviewboard review request status.)
Flags: needinfo?(kmaglione+bmo)
Ugh. Sorry, I already addressed that. I guess reviewboard doesn't send notifications of the update when the review is still pending.
Flags: needinfo?(kmaglione+bmo)

Comment 12

a year ago
mozreview-review
Comment on attachment 8855485 [details]
Bug 1352827: Handle potential null reference during cleanup by CC.

https://reviewboard.mozilla.org/r/127316/#review132414
Attachment #8855485 - Flags: review?(hsivonen) → review+
(In reply to Kris Maglione [:kmag] from comment #11)
> Ugh. Sorry, I already addressed that. I guess reviewboard doesn't send
> notifications of the update when the review is still pending.

OK. Sorry. ReviewBoard UI continues to confuse me in both requestor and requestee roles.
(In reply to Henri Sivonen (:hsivonen) from comment #13)
> OK. Sorry. ReviewBoard UI continues to confuse me in both requestor and
> requestee roles.

Don't worry, you're not the only one...

Comment 15

a year ago
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dd2c353c32bc
Handle potential null reference during cleanup by CC. r=hsivonen

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd2c353c32bc
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox-esr52: --- → unaffected
Blocks: 1396527
You need to log in before you can comment on or make changes to this bug.