Closed Bug 831472 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla21
Tracking Status
firefox18 --- wontfix
firefox19 + verified
firefox20 + verified
firefox21 + verified
firefox-esr10 --- wontfix
firefox-esr17 --- wontfix
b2g18 - wontfix

People

(Reporter: gkw, Assigned: gkw)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
+++ 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
Attachment #703004 - Flags: review?(bugs)
Attached patch patch v2Splinter Review
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.
Attachment #703004 - Attachment is obsolete: true
Attachment #703004 - Flags: review?(bugs)
Attachment #703017 - Flags: review?(bugs)
Attachment #703017 - Flags: review?(bugs) → review+
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
Blocks: 486200
Crash Signature: [@ nsGlobalWindow::GetInnerScreenRect] → [@ nsGlobalWindow::GetInnerScreenRect] [@ nsGlobalWindow::GetInnerScreenRect()]
https://hg.mozilla.org/mozilla-central/rev/74dc65a5401b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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.
Attachment #703017 - Flags: approval-mozilla-aurora?
[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();
Attachment #703597 - Flags: review?(bugs)
Attachment #703597 - Flags: approval-mozilla-beta?
Attachment #703597 - Flags: approval-mozilla-esr17?
Attachment #703597 - Flags: approval-mozilla-b2g18?
Attachment #703597 - Flags: review?(bugs) → review+
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.
Attachment #703597 - Attachment description: backported patch for mozilla-beta (19) and prior → backported version of patch v2 for mozilla-beta (19) and prior
Comment on attachment 703017 [details] [diff] [review]
patch v2

null check, approved for aurora.
Attachment #703017 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #703597 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-beta/rev/a5aa40534b52

This is also in my queue to land on Aurora once it reopens.
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.
Attachment #703597 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18-
Revisiting comment 8, this does not seem to fall in the ESR landing criteria range so minusing & marking it WONTFIX at this time.
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.
Attachment #703597 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.