Closed Bug 1256419 Opened 8 years ago Closed 8 years ago

crashes in nsDOMWindowList::EnsureFresh

Categories

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

Unspecified
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- unaffected
firefox47 + fixed
firefox48 --- fixed

People

(Reporter: dbaron, Assigned: bzbarsky)

References

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(2 files)

[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.
Flags: needinfo?(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.
Assignee: nobody → bzbarsky
Flags: needinfo?(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.
Attachment #8730369 - Flags: approval-mozilla-aurora?
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.
Attachment #8730369 - Flags: review?(peterv) → review+
https://hg.mozilla.org/mozilla-central/rev/f75d479d9d51
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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 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+
Attachment #8730369 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.