PepperOutputProtectionMessageFilter: set window_ only when client_id_ is valid.

PepperOutputProtectionMessageFilter: set window_ only when client_id_ is valid.

TBR=kcwu@chromium.org
BUG=484857
TEST=No crash now.

Committed: https://crrev.com/246e2d1ea8c141751689b28538e3163d254efca8
Cr-Commit-Position: refs/heads/master@{#330494}

[xhwang, 2015-05-05 23:19:58]

xhwang@chromium.org changed reviewers:
+ kcwu@chromium.org

[xhwang, 2015-05-05 23:19:59]

PTAL

https://codereview.chromium.org/1122353002/diff/1/chrome/browser/renderer_hos...
File
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc
(right):

https://codereview.chromium.org/1122353002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:162:
GetWindow()->RemoveObserver(this);
See l.171. Even with the fix there, the window_ could be invalid here even
though it's non-null. I don't know why OnWindowDestroying() isn't called
though...

https://codereview.chromium.org/1122353002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:171:
if (!rfh || !GetCurrentDisplayId(rfh, &display_id_))
This fixes the crash in the bug.

[kcwu, 2015-05-07 06:58:12]

This CL basically look good but I am not very comfortable because we don't know
the reason of window_ failure.

https://codereview.chromium.org/1122353002/diff/1/chrome/browser/renderer_hos...
File
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc
(right):

https://codereview.chromium.org/1122353002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:162:
GetWindow()->RemoveObserver(this);
On 2015/05/05 23:19:58, xhwang wrote:
> See l.171. Even with the fix there, the window_ could be invalid here even
> though it's non-null. I don't know why OnWindowDestroying() isn't called
> though...

After fix in l.171, is the same reproduce steps trigger bug of invalid window_ ?

[xhwang, 2015-05-07 07:19:21]

https://codereview.chromium.org/1122353002/diff/1/chrome/browser/renderer_hos...
File
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc
(right):

https://codereview.chromium.org/1122353002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:162:
GetWindow()->RemoveObserver(this);
On 2015/05/07 06:58:11, kcwu wrote:
> On 2015/05/05 23:19:58, xhwang wrote:
> > See l.171. Even with the fix there, the window_ could be invalid here even
> > though it's non-null. I don't know why OnWindowDestroying() isn't called
> > though...
> 
> After fix in l.171, is the same reproduce steps trigger bug of invalid window_
?

Exactly.

What exactly is this |window_|? Since it's associated with the RFH, I guess it's
not surprising that when RFH is gone, the window is also destroyed. But as I
said I don't know why OnWindowDestroying() isn't called.

[kcwu, 2015-05-07 09:18:46]

https://codereview.chromium.org/1122353002/diff/1/chrome/browser/renderer_hos...
File
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc
(right):

https://codereview.chromium.org/1122353002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:182:
GetWindow()->AddObserver(this);
Does following code fix your crash? If so, I would prefer not changing to
GetWindow() everywhere, especially line 161.

line 174
  aura::Window* window = rfh->GetNativeView();
  if (!window)
    return ui::DisplayConfigurator::kInvalidClientId;

line 181
  if (client_id_ != ui::DisplayConfigurator::kInvalidClientId) {
    window->AddObserver(this);
    window_ = window;
  }

[xhwang, 2015-05-15 23:03:16]

Your advice is working. PTAL again.

https://codereview.chromium.org/1122353002/diff/1/chrome/browser/renderer_hos...
File
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc
(right):

https://codereview.chromium.org/1122353002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:182:
GetWindow()->AddObserver(this);
On 2015/05/07 09:18:46, kcwu wrote:
> Does following code fix your crash? If so, I would prefer not changing to
> GetWindow() everywhere, especially line 161.
> 
> line 174
>   aura::Window* window = rfh->GetNativeView();
>   if (!window)
>     return ui::DisplayConfigurator::kInvalidClientId;
> 
> line 181
>   if (client_id_ != ui::DisplayConfigurator::kInvalidClientId) {
>     window->AddObserver(this);
>     window_ = window;
>   }

This is working. Thank you!

[kcwu, 2015-05-16 12:52:17]

lgtm

[xhwang, 2015-05-18 16:53:25]

xhwang@chromium.org changed reviewers:
+ bbudge@chromium.org

[xhwang, 2015-05-18 16:53:25]

bbudge@chromium.org: Please OWNERS review!

[xhwang, 2015-05-19 00:26:24]

xhwang@chromium.org changed reviewers:
+ raymes@chromium.org

[xhwang, 2015-05-19 00:26:24]

+raymes for the OWNERS review in case bbudge is OOO.

[raymes, 2015-05-19 01:04:59]

Is it difficult to add a test for the crash?

https://codereview.chromium.org/1122353002/diff/40001/chrome/browser/renderer...
File
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc
(right):

https://codereview.chromium.org/1122353002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:182:
window_->AddObserver(this);
This seems weird: are we adding the observer to the old window? I don't have any
context about this code but it looks very strange. A comment could be
worthwhile.

[xhwang, 2015-05-19 04:02:13]

comments addressed

[xhwang, 2015-05-19 05:18:58]

On 2015/05/19 01:04:59, raymes wrote:
> Is it difficult to add a test for the crash?

This crash only happens on a Linux ChromeOS build and sometimes happens when I
close the tab playing Netflix. I don't really see an easy way to write a
reliable test to test this. Also, currently there are no unit tests for this
file. It might be easier to unit test it after my other CL lands
(https://codereview.chromium.org/1139923006/).

[xhwang, 2015-05-19 05:19:16]

raymes: Sorry for the rebase noise. PTAL again.

https://codereview.chromium.org/1122353002/diff/40001/chrome/browser/renderer...
File
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc
(right):

https://codereview.chromium.org/1122353002/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:182:
window_->AddObserver(this);
On 2015/05/19 01:04:59, raymes wrote:
> This seems weird: are we adding the observer to the old window? I don't have
any
> context about this code but it looks very strange. A comment could be
> worthwhile.

Good catch! It's my bad (typo). Fixed.

[raymes, 2015-05-19 06:11:34]

lgtm

Please consider adding a unittest if possible :)

[xhwang, 2015-05-19 07:40:38]

rebase only

[xhwang, 2015-05-19 07:47:42]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-05-19 07:47:43]

The patchset sent to the CQ was uploaded after l-g-t-m from kcwu@chromium.org,
raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1122353002/#ps80001
(title: "rebase only")

[commit-bot: I haz the power, 2015-05-19 07:48:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1122353002/80001

[commit-bot: I haz the power, 2015-05-19 08:28:15]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-19 08:29:15]

Patchset 5 (id:??) landed as
https://crrev.com/246e2d1ea8c141751689b28538e3163d254efca8
Cr-Commit-Position: refs/heads/master@{#330494}