Closed Bug 1352827 Opened 4 years ago Closed 4 years ago

Crash in UnblockParsingPromiseHandler::MaybeUnblockParser

Categories

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

Unspecified
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: calixte, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, Whiteboard: [clouseau])

Crash Data

Attachments

(1 file)

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 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 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 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.
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 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...
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dd2c353c32bc
Handle potential null reference during cleanup by CC. r=hsivonen
https://hg.mozilla.org/mozilla-central/rev/dd2c353c32bc
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.