1139845 - Fix ChromeProcessController crash when window is immediately closed

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

If a widget is shut down too fast, ChromeProcessController::InitializeRoot will still fire later and assumes there's still a view.

Comment 2

[Botond Ballo [:botond]]

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

Comment 3

[Mats Palmgren (inactive)]

Don't we need to update things like "GetPresShell()->GetDocument()" now that
GetPresShell() can return null?

Comment 4

[Botond Ballo [:botond]]

Attachment: Fix

Yes, you're right. Updated patch to do so.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 8574053 [details] [diff] [review]
Fix

Thanks.

Comment 6

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking crash.

Comment 7

[Botond Ballo [:botond]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c8a064db03a

Comment 8

[Botond Ballo [:botond]]

(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.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4400a4a2de00

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: follow-up

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.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4400a4a2de00

Comment 12

[Botond Ballo [:botond]]

(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?

Comment 13

[Mats Palmgren (inactive)]

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? :-)

Comment 14

[Mats Palmgren (inactive)]

(I looked at the file in my *beta* source tree that is, which I assume is 37 at this point)

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

(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. :)

Comment 16

[Botond Ballo [:botond]]

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.

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 8575549 [details] [diff] [review]
follow-up

See previous comment.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47f212881821

Comment 21

[Botond Ballo [:botond]]

(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)!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47f212881821