1256419 - crashes in nsDOMWindowList::EnsureFresh

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-b6ab14ac-1f9e-4b4b-9a72-12d092160314.
=============================================================

There have been a decent number of crashes in nsDOMWindowList::EnsureFresh.  Not sure if it's quite enough to qualify as a topcrash, but it's at least close.

They first occurred in builds from 2016-02-10.  See query at:
https://crash-stats.mozilla.com/signature/?product=Firefox&release_channel=nightly&platform=Windows&date=%3E%3D2016-01-01&signature=nsDOMWindowList%3A%3AEnsureFresh&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1#aggregations
(40 crashes so far on nightly).

At first glance, it looks like a missing null-check in code added in bug 1245554, although I didn't actually do the math to double-check that an 0x10 crash address matches childWindows being null.

Comment 1

[Boris Zbarsky [:bzbarsky]]

0x10 (or 0x8 in 32-bit builds, which also shows up in the reports) is in fact consistent with that: there's one word of vtable, then one word of refcount, then the mDocShellNode member we're trying to read.

And yes, this needs a null-check.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Null-check our nsDOMWindowList before trying to get its length

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Same thing as diff -w

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8730369 [details] [diff] [review]
Null-check our nsDOMWindowList before trying to get its length

Approval Request Comment
[Feature/regressing bug #]: Bug 1245554
[User impact if declined]: Crashes sometimes
[Describe test coverage new/current, TreeHerder]: we have good coverage for the
   non-crashing case.
[Risks and why]: Extremely low risk.  Just skips some code if we don't have the
   right object around anymore.
[String/UUID change made/needed]: None.

Comment 5

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked since it's a top crasher.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8730369 [details] [diff] [review]
Null-check our nsDOMWindowList before trying to get its length

Just got this.
Looks like the old code did have a null check.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f75d479d9d51

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I want to wait to see crash-stats on this one confirm that the fix is helping before uplifting it even though the fix is a simple not null check.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8730369 [details] [diff] [review]
Null-check our nsDOMWindowList before trying to get its length

Crash-stats does not show any occurrences of this signature on Nightly 48 since 03-15 build, looks good. Aurora47+

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dcc8b840bd8f