Route Pepper MediaSession messages to frames

Route Pepper MediaSession messages to frames

This CL fixes an issue that SetPepperVolume (coming from
MediaSession) is only sent to the top-level frame. Instead, the
message should be handled by the frames which created the Pepper
instance. The issue can cause a crash if there is a Pepper
instance playing sound in a cross-origin iframe while
site-isolation is enabled.

Now we use pair <RenderFrameHost, pp_instance> as player id so
that SetPepperVolume can be sent to the correct frame.

BUG=670273
Committed: https://crrev.com/7b6f4f00951c96ba468bd8ad554fda1065bacfa6
Cr-Commit-Position: refs/heads/master@{#437079}

[Zhiqiang Zhang (Slow), 2016-12-05 16:01:30]

Description was changed from

==========
Route Pepper MediaSession messages to frames

BUG=
==========

to

==========
Route Pepper MediaSession messages to frames

This CL fixes an issue that SetPepperVolume (coming from
MediaSession) is only sent to the top-level frame. Instead, the
message should be handled by the frames which created the Pepper
instance. The issue can cause a crash if there is a Pepper
instance playing sound in a cross-origin iframe while
site-isolation is enabled.

Now we use pair <RenderFrameHost, pp_instance> as player id so
that SetPepperVolume can be sent to the correct frame.

BUG=670273
==========

[Zhiqiang Zhang (Slow), 2016-12-05 16:14:07]

zqzhang@chromium.org changed reviewers:
+ avayvod@chromium.org

[Zhiqiang Zhang (Slow), 2016-12-05 16:14:08]

PTAL

[whywhat, 2016-12-06 01:37:31]

lgtm % the WCImpl change

https://codereview.chromium.org/2551093002/diff/20001/content/browser/media/s...
File content/browser/media/session/pepper_playback_observer.cc (right):

https://codereview.chromium.org/2551093002/diff/20001/content/browser/media/s...
content/browser/media/session/pepper_playback_observer.cc:11: #include
"content/browser/web_contents/web_contents_impl.h"
nit: Is this header necessary? Seems like the only use for contents_ is to call
MediaSessionImpl::Get() which should be covered by media_session_impl.h

https://codereview.chromium.org/2551093002/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2551093002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:772:
IPC_BEGIN_MESSAGE_MAP_WITH_PARAM(WebContentsImpl, message,
nit: change WebContentsImpl to RenderFrameHostImpl?

https://codereview.chromium.org/2551093002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3778: render_frame_host,
pp_instance);
Hm, it seems like you don't need to change the map above and just pass
render_frame_message_source_ here (which could be null in theory?). See the
comment before the original IPC_MESSAGE_MAP.

[Zhiqiang Zhang (Slow), 2016-12-06 12:05:56]

https://codereview.chromium.org/2551093002/diff/20001/content/browser/media/s...
File content/browser/media/session/pepper_playback_observer.cc (right):

https://codereview.chromium.org/2551093002/diff/20001/content/browser/media/s...
content/browser/media/session/pepper_playback_observer.cc:11: #include
"content/browser/web_contents/web_contents_impl.h"
On 2016/12/06 01:37:31, whywhat_OOO_till_Mon_Nov_28 wrote:
> nit: Is this header necessary? Seems like the only use for contents_ is to
call
> MediaSessionImpl::Get() which should be covered by media_session_impl.h

Actually we only need to store WebContents* in PepperPlaybackObserver, so
s/WebContentsImpl/WebContents in the header. This include is no longer necessary
:)

https://codereview.chromium.org/2551093002/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2551093002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:772:
IPC_BEGIN_MESSAGE_MAP_WITH_PARAM(WebContentsImpl, message,
On 2016/12/06 01:37:31, whywhat_OOO_till_Mon_Nov_28 wrote:
> nit: change WebContentsImpl to RenderFrameHostImpl?

No longer applicable, this change is reverted.

https://codereview.chromium.org/2551093002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3778: render_frame_host,
pp_instance);
On 2016/12/06 01:37:31, whywhat_OOO_till_Mon_Nov_28 wrote:
> Hm, it seems like you don't need to change the map above and just pass
> render_frame_message_source_ here (which could be null in theory?). See the
> comment before the original IPC_MESSAGE_MAP.

Didn't realize that. Thanks for pointing out. Done

[Zhiqiang Zhang (Slow), 2016-12-06 12:07:49]

zqzhang@chromium.org changed reviewers:
+ piman@chromium.org

[Zhiqiang Zhang (Slow), 2016-12-06 12:07:50]

+piman: web_contents_impl.cc

[piman, 2016-12-06 21:26:12]

https://codereview.chromium.org/2551093002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2551093002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3764:
render_frame_message_source_, pp_instance);
I am somewhat concerned that render_frame_message_source_ refers to a
RenderFrameHost that could get destroyed later, but I don't think we have a
guarantee that OnPepperInstanceDeleted will be called before then (since a
compromised renderer can send - or not send - any message it wants).

Should the PepperPlaybackObserver also track the RenderFrameHost lifetime (e.g.
with a WebContentsObserver)?

[Zhiqiang Zhang (Slow), 2016-12-07 16:31:54]

Patchset #4 (id:60001) has been deleted

[Zhiqiang Zhang (Slow), 2016-12-07 16:36:21]

PTAL

Addressed piman's comments (improved cleaning up when RenderFrame/WebContents is
destroyed)

https://codereview.chromium.org/2551093002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2551093002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3764:
render_frame_message_source_, pp_instance);
On 2016/12/06 21:26:12, piman wrote:
> I am somewhat concerned that render_frame_message_source_ refers to a
> RenderFrameHost that could get destroyed later, but I don't think we have a
> guarantee that OnPepperInstanceDeleted will be called before then (since a
> compromised renderer can send - or not send - any message it wants).
> 
> Should the PepperPlaybackObserver also track the RenderFrameHost lifetime
(e.g.
> with a WebContentsObserver)?

Thanks for pointing out. Done.

[whywhat, 2016-12-07 18:09:25]

https://codereview.chromium.org/2551093002/diff/100001/content/browser/media/...
File content/browser/media/session/pepper_playback_observer.cc (right):

https://codereview.chromium.org/2551093002/diff/100001/content/browser/media/...
content/browser/media/session/pepper_playback_observer.cc:49:
players_to_remove.push_back(player_played_sound.first);
nit: I think you could use the erase(it++) idiom here to avoid looking for this
element again in PepperInstanceDeleted and call PepperStopsPlayback directly
from the loop?

[piman, 2016-12-07 19:27:06]

lgtm

[Zhiqiang Zhang (Slow), 2016-12-07 19:39:53]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[Zhiqiang Zhang (Slow), 2016-12-07 19:40:23]

https://codereview.chromium.org/2551093002/diff/100001/content/browser/media/...
File content/browser/media/session/pepper_playback_observer.cc (right):

https://codereview.chromium.org/2551093002/diff/100001/content/browser/media/...
content/browser/media/session/pepper_playback_observer.cc:49:
players_to_remove.push_back(player_played_sound.first);
On 2016/12/07 18:09:25, whywhat wrote:
> nit: I think you could use the erase(it++) idiom here to avoid looking for
this
> element again in PepperInstanceDeleted and call PepperStopsPlayback directly
> from the loop?

Thanks! Done.

[commit-bot: I haz the power, 2016-12-07 19:40:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 20:13:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 20:13:07]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Zhiqiang Zhang (Slow), 2016-12-07 21:13:34]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-12-07 21:13:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2551093002/#ps140001
(title: "fixed Android build")

[commit-bot: I haz the power, 2016-12-07 21:14:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 22:10:27]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481145214003230,
"parent_rev": "91521ef94aea6470ae762da97c1aaaaa0b006b49", "commit_rev":
"044fe7b00f3bda954b68eff30820f5cde47de3c5"}

[commit-bot: I haz the power, 2016-12-07 22:10:50]

Description was changed from

==========
Route Pepper MediaSession messages to frames

This CL fixes an issue that SetPepperVolume (coming from
MediaSession) is only sent to the top-level frame. Instead, the
message should be handled by the frames which created the Pepper
instance. The issue can cause a crash if there is a Pepper
instance playing sound in a cross-origin iframe while
site-isolation is enabled.

Now we use pair <RenderFrameHost, pp_instance> as player id so
that SetPepperVolume can be sent to the correct frame.

BUG=670273
==========

to

==========
Route Pepper MediaSession messages to frames

This CL fixes an issue that SetPepperVolume (coming from
MediaSession) is only sent to the top-level frame. Instead, the
message should be handled by the frames which created the Pepper
instance. The issue can cause a crash if there is a Pepper
instance playing sound in a cross-origin iframe while
site-isolation is enabled.

Now we use pair <RenderFrameHost, pp_instance> as player id so
that SetPepperVolume can be sent to the correct frame.

BUG=670273
==========

[commit-bot: I haz the power, 2016-12-07 22:10:51]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-12-07 22:13:19]

Description was changed from

==========
Route Pepper MediaSession messages to frames

This CL fixes an issue that SetPepperVolume (coming from
MediaSession) is only sent to the top-level frame. Instead, the
message should be handled by the frames which created the Pepper
instance. The issue can cause a crash if there is a Pepper
instance playing sound in a cross-origin iframe while
site-isolation is enabled.

Now we use pair <RenderFrameHost, pp_instance> as player id so
that SetPepperVolume can be sent to the correct frame.

BUG=670273
==========

to

==========
Route Pepper MediaSession messages to frames

This CL fixes an issue that SetPepperVolume (coming from
MediaSession) is only sent to the top-level frame. Instead, the
message should be handled by the frames which created the Pepper
instance. The issue can cause a crash if there is a Pepper
instance playing sound in a cross-origin iframe while
site-isolation is enabled.

Now we use pair <RenderFrameHost, pp_instance> as player id so
that SetPepperVolume can be sent to the correct frame.

BUG=670273

Committed: https://crrev.com/7b6f4f00951c96ba468bd8ad554fda1065bacfa6
Cr-Commit-Position: refs/heads/master@{#437079}
==========

[commit-bot: I haz the power, 2016-12-07 22:13:20]

Patchset 7 (id:??) landed as
https://crrev.com/7b6f4f00951c96ba468bd8ad554fda1065bacfa6
Cr-Commit-Position: refs/heads/master@{#437079}