Closed Bug 1139845 Opened 5 years ago Closed 5 years ago

Fix ChromeProcessController crash when window is immediately closed

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 --- unaffected
firefox38 + fixed
firefox39 + fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
If a widget is shut down too fast, ChromeProcessController::InitializeRoot will still fire later and assumes there's still a view.
Attachment #8573206 - Flags: review?(botond)
Attachment #8573206 - Flags: review?(botond) → review+
Duplicate of this bug: 1140201
Carrying over tracking flags from duplicate bug 1140201.

[Tracking Requested - why for this release]:
we should fix this crash before it hits the release channel
Don't we need to update things like "GetPresShell()->GetDocument()" now that
GetPresShell() can return null?
Flags: needinfo?(botond)
Attached patch FixSplinter Review
Yes, you're right. Updated patch to do so.
Attachment #8573206 - Attachment is obsolete: true
Flags: needinfo?(botond)
Attachment #8574053 - Flags: review?(mats)
Comment on attachment 8574053 [details] [diff] [review]
Fix

Thanks.
Attachment #8574053 - Flags: review?(mats) → review+
(In reply to Botond Ballo [:botond] from comment #7)
> Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c8a064db03a

All green! Setting checkin-needed as the tree is closed.
Keywords: checkin-needed
Attached patch follow-upSplinter Review
This still crashed on try, so one more follow-up fix: looks like even if we get a document, its document element could be gone.
Attachment #8575549 - Flags: review?(botond)
https://hg.mozilla.org/mozilla-central/rev/4400a4a2de00
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Attachment #8575549 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #2)
> Carrying over tracking flags from duplicate bug 1140201.

I didn't scrutinize the tracking flags when I carried them over, but now that I look at them again, I don't think this needs to be tracking Firefox 37: the bug which introduced the crash is bug 1124452, which landed in 38, and was uplifted to the b2g37 branch, but not to 37 itself. Mats, is there something I missed, or can we remove the tracking-firefox37 flag?
Flags: needinfo?(mats)
Fwiw, I looked at gfx/layers/apz/util/ChromeProcessController.cpp and I don't see the changes
from bug 1124452 there so I think you're right that that 37 isn't affected, except for b2g.
(I guess you should ask approval for b2g37 on the patches in this bug though?)

lmandel perhaps wants to tripple-check that? :-)
Flags: needinfo?(mats) → needinfo?(lmandel)
(I looked at the file in my *beta* source tree that is, which I assume is 37 at this point)
(In reply to Mats Palmgren (:mats) from comment #13)
> Fwiw, I looked at gfx/layers/apz/util/ChromeProcessController.cpp and I
> don't see the changes
> from bug 1124452 there so I think you're right that that 37 isn't affected,
> except for b2g.
> lmandel perhaps wants to tripple-check that? :-)

Confirmed. I checked Socorro and don't see any crash reports for 37.


(In reply to Mats Palmgren (:mats) from comment #14)
> (I looked at the file in my *beta* source tree that is, which I assume is 37
> at this point)

And indeed Beta is currently 37. :)
Flags: needinfo?(lmandel)
Comment on attachment 8574053 [details] [diff] [review]
Fix

Requesting approval to uplift to aurora (38) and b2g37, as these are the branches the regressing bug is on.

Approval Request Comment
[Feature/regressing bug #]:
  Bug 1124452.

[User impact if declined]:
  Crashes: https://crash-stats.mozilla.com/report/list?signature=nsView%3A%3AGetPresShell%28%29#tab-reports

[Describe test coverage new/current, TreeHerder]:
  On m-c since 2015-03-10.

[Risks and why]: 
  Low, this is an obvious null pointer dereference fix.

[String/UUID change made/needed]:
  None.
Attachment #8574053 - Flags: approval-mozilla-b2g37?
Attachment #8574053 - Flags: approval-mozilla-aurora?
Comment on attachment 8575549 [details] [diff] [review]
follow-up

See previous comment.
Attachment #8575549 - Flags: approval-mozilla-b2g37?
Attachment #8575549 - Flags: approval-mozilla-aurora?
Attachment #8574053 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8575549 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/743955e3644c

I folded the follow-up into the first patch. That said, it appears to have not landed on trunk yet?
Flags: needinfo?(dvander)
Attachment #8574053 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(dvander)
Attachment #8575549 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #18)
> I folded the follow-up into the first patch. That said, it appears to have
> not landed on trunk yet?

Indeed, I didn't realize the follow-up hasn't landed yet when I requested the approval. Thanks for catching that (and for landing it)!
You need to log in before you can comment on or make changes to this bug.