1304457 - RemoteContentController::Destroy should set mCanSend = false

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P2)

Description

[Kartikaya Gupta (email:[e-mail])]

Quoting bug 1272942 comment 33:

> I reproduced in rr and it looks like mCanSend should be getting set to false
> in RemoteContentController::Destroy(). When that function calls SendDestroy,
> it sends a destroy message to APZChild which promptly deletes itself during
> the processing of that message. So the parent side shouldn't be sending any
> more messages to the child after it has called SendDestroy().

Also while I'm here I'm going to fix bug 1298954 comment 8:

> At least one thing wrong with the code in RemoteContentController is that it
> doesn't have mCanSend checks on a few functions - including
> SetScrollingRootContent.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Regression from bug 1289650.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1304457 - Ensure that RemoteContentController doesn't try to send any messages after it has sent the Destroy message.

Review commit: https://reviewboard.mozilla.org/r/80154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80154/

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1304457 - Add some missing mCanSend checks.

Review commit: https://reviewboard.mozilla.org/r/80156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80156/

Comment 4

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

Comment on attachment 8793413 [details]
Bug 1304457 - Ensure that RemoteContentController doesn't try to send any messages after it has sent the Destroy message.

https://reviewboard.mozilla.org/r/80154/#review78822

Comment 5

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

Comment on attachment 8793414 [details]
Bug 1304457 - Add some missing mCanSend checks.

https://reviewboard.mozilla.org/r/80156/#review78824

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c3920483acd
Ensure that RemoteContentController doesn't try to send any messages after it has sent the Destroy message. r=dvander
https://hg.mozilla.org/integration/autoland/rev/ab5ccadf750e
Add some missing mCanSend checks. r=dvander

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c3920483acd
https://hg.mozilla.org/mozilla-central/rev/ab5ccadf750e

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8793413 [details]
Bug 1304457 - Ensure that RemoteContentController doesn't try to send any messages after it has sent the Destroy message.

Approval Request Comment
[Feature/regressing bug #]: bug 1289650
[User impact if declined]: sporadic crashes
[Describe test coverage new/current, TreeHerder]: some automated tests were intermittently crashing because of this bug (see bug 1272942), so this code is exercised in automation
[Risks and why]: pretty low risk, code is well understood and the bug appears to be have been a simple oversight
[String/UUID change made/needed]: none

Comment 9

[Gerry Chang [:gchang]]

Comment on attachment 8793413 [details]
Bug 1304457 - Ensure that RemoteContentController doesn't try to send any messages after it has sent the Destroy message.

This patch fixes a regression. Take it in 51 aurora.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/68cf1c5b64ec
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/4dd6247f6710

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Can you please request Beta approval for this given bug 1272942 comment 40? Thanks!

Comment 12

[Kartikaya Gupta (email:[e-mail])]

This patch won't apply to beta. As far as I can tell the beta code doesn't suffer from this problem. I commented on bug 1272942.