831472 - Crash [@ nsGlobalWindow::GetInnerScreenRect()] due to lack of null check

Status: VERIFIED FIXED

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

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: patch

+++ This bug was initially created as a clone of Bug #830948 +++

Crash signature: bp-edf7e923-31c8-432f-a7c0-619402130116

I did some more poking via crash-stats and spotted some more crashes in nsGlobalWindow::GetInnerScreenRect()

An example crashing line is at http://hg.mozilla.org/releases/mozilla-release/annotate/8efe34fa2289/dom/base/nsGlobalWindow.cpp#l3889

smaug mentions on #developers that FlushPendingNotifications might also have caused mDocshell to become null.

The code in question was added in bug 486200 via http://hg.mozilla.org/mozilla-central/rev/42079bb031e1

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: patch v2

smaug suggests adding another !mDocShell check instead since the nsGlobalWindow::GetInnerScreenRect() function returns nsRect instead, and not nsresult.

There also some formatting changes in the same function.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I made a small change to the patch landing comment, instead of "Add null check..." to "Add another null check..."

https://hg.mozilla.org/integration/mozilla-inbound/rev/74dc65a5401b

Comment 3

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/74dc65a5401b

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 703017 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): likely bug 486200, after an inspection via `hg blame`.
User impact if declined: crashes
Testing completed: landed on m-c recently
Risk to taking this patch (and alternatives if risky): null check
String or UUID changes made by this patch: N.A.

This patch applies on mozilla-aurora (20) but needs to be backported to mozilla-beta (19) and mozilla-esr17 and b2g18.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: backported version of patch v2 for mozilla-beta (19) and prior

[Approval Request Comment]
Bug caused by (feature/regressing bug #): likely bug 486200, after an inspection via `hg blame`.
User impact if declined: crashes
Testing completed: landed on m-c recently
Risk to taking this patch (and alternatives if risky): null check
String or UUID changes made by this patch: N.A.


Relevant interdiff between the original patch and this backported patch is in the context:

31,32c35
<    nsCOMPtr<nsIPresShell> presShell;
<    mDocShell->GetPresShell(getter_AddRefs(presShell));
---
>    nsCOMPtr<nsIPresShell> presShell = mDocShell->GetPresShell();

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This crash probably isn't likely to warrant fixing on release and esr10 (it isn't a topcrash, and esr17 is out), so WONTFIX'ing.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 703017 [details] [diff] [review]
patch v2

null check, approved for aurora.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 703597 [details] [diff] [review]
backported version of patch v2 for mozilla-beta (19) and prior

approving for beta - I don't see why we'd take this on esr since it doesn't meet esr landing criteria. Will check with Bhavana on her reasons for tracking and come back later with decision.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a5aa40534b52

This is also in my queue to land on Aurora once it reopens.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/686eab2fdb3a

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 703597 [details] [diff] [review]
backported version of patch v2 for mozilla-beta (19) and prior

Not taking this on b2g18 unless this fixes a common problem for b2g.

Comment 12

[bhavana bajaj [:bajaj]]

Revisiting comment 8, this does not seem to fall in the ESR landing criteria range so minusing & marking it WONTFIX at this time.

Comment 13

[bhavana bajaj [:bajaj]]

Comment on attachment 703597 [details] [diff] [review]
backported version of patch v2 for mozilla-beta (19) and prior

Please check comment #12, i had forgotten to a- the patch earlier while commenting.

Comment 14

[Ada [:adalucinet]]

Verified fixed based on the crash reports:

[@ nsGlobalWindow::GetInnerScreenRect] signature: 0 crashes in the last week
https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=nsGlobalWindow%3A%3AGetInnerScreenRect&reason_type=contains&date=02%2F07%2F2013%2013%3A20%3A12&range_value=1&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=nsGlobalWindow%3A%3AGetInnerScreenRect#reports

[@ nsGlobalWindow::GetInnerScreenRect()] signature: 1 crash in the last week
https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=nsGlobalWindow%3A%3AGetInnerScreenRect%28%29&reason_type=contains&date=02%2F07%2F2013%2013%3A20%3A10&range_value=1&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=nsGlobalWindow%3A%3AGetInnerScreenRect%28%29#reports

Comment 15

[Ada [:adalucinet]]

Verified as fixed based on the crash reports below:

[@ nsGlobalWindow::GetInnerScreenRect] signature: 0 crashes with FF 20 builds
https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=nsGlobalWindow%3A%3AGetInnerScreenRect&reason_type=contains&date=03%2F28%2F2013%2010%3A47%3A07&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=nsGlobalWindow%3A%3AGetInnerScreenRect#reports

[@ nsGlobalWindow::GetInnerScreenRect()] signature: 2 crashes with FF 20 builds (beta 1 and beta 5)
https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=nsGlobalWindow%3A%3AGetInnerScreenRect&reason_type=contains&date=03%2F28%2F2013%2010%3A47%3A07&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=nsGlobalWindow%3A%3AGetInnerScreenRect%28%29#reports

Comment 16

[Ada [:adalucinet]]

Verified as fixed on Firefox 21 beta 2 based on the crash reports below:

[@ nsGlobalWindow::GetInnerScreenRect] signature: 0 crashes on FF 21 builds
https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=nsGlobalWindow%3A%3AGetInnerScreenRect&reason_type=contains&date=04%2F09%2F2013%2009%3A47%3A23&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=nsGlobalWindow%3A%3AGetInnerScreenRect#reports

[@ nsGlobalWindow::GetInnerScreenRect()] signature: 0 crashes on FF 21 builds
https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=nsGlobalWindow%3A%3AGetInnerScreenRect&reason_type=contains&date=04%2F09%2F2013%2009%3A47%3A23&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=nsGlobalWindow%3A%3AGetInnerScreenRect%28%29#reports