1352827 - Crash in UnblockParsingPromiseHandler::MaybeUnblockParser

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P2)

Description

[Calixte Denizet (:calixte)]

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

Comment 2

[Kris Maglione [:kmag]]

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.

Comment 3

[Andrew Overholt [:overholt]]

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?

Comment 4

[Kris Maglione [:kmag]]

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.

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1352827: Handle potential null reference during cleanup by CC.

Review commit: https://reviewboard.mozilla.org/r/127316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127316/

Comment 6

[Henri Sivonen (:hsivonen)]

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?

Comment 7

[Kris Maglione [:kmag]]

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

[Henri Sivonen (:hsivonen)]

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 9

[Kris Maglione [:kmag]]

Comment on attachment 8855485 [details]
Bug 1352827: Handle potential null reference during cleanup by CC.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127316/diff/1-2/

Comment 10

[Henri Sivonen (:hsivonen)]

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

Comment 11

[Kris Maglione [:kmag]]

Ugh. Sorry, I already addressed that. I guess reviewboard doesn't send notifications of the update when the review is still pending.

Comment 12

[Henri Sivonen (:hsivonen)]

Comment on attachment 8855485 [details]
Bug 1352827: Handle potential null reference during cleanup by CC.

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

Comment 13

[Henri Sivonen (:hsivonen)]

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

Comment 14

[Kris Maglione [:kmag]]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd2c353c32bc
Handle potential null reference during cleanup by CC. r=hsivonen

Comment 16

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/dd2c353c32bc