Allow a main interstitial frame to act as an ExtensionMessagePort opener

Allow a main interstitial frame to act as an ExtensionMessagePort opener

BUG=575234, 578828
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/42079f3f4b4f940a77392ebda383ea943aeef511
Cr-Commit-Position: refs/heads/master@{#371969}

[robwu, 2016-01-24 20:41:36]

Description was changed from

==========
Allow a main interstitial frame to act as an ExtensionMessagePort opener

BUG=575234,578828
==========

to

==========
Allow a main interstitial frame to act as an ExtensionMessagePort opener

BUG=575234,578828
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[robwu, 2016-01-24 22:32:30]

rob@robwu.nl changed reviewers:
+ creis@chromium.org, mek@chromium.org

[robwu, 2016-01-24 22:32:31]

Charlie: content/

Marijn: chrome/

[Charlie Reis, 2016-01-25 21:47:38]

[+rdevlin.cronin, nick to CC]

Thanks for coming up with a fix here.  I'm not sure I understand if there's
compelling use cases for message ports on interstitials or if we're just
preserving some unintentional support, but it probably makes sense to fix this
crash and debate that later.

My main question below is about how we use |tab| after getting it from the
interstitial, because we can't assume that RFH is a normal one.  I see that you
listen to DidDetachInterstitialPage for cleanup, but I don't know how else the
tracked frame is used.

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/extension_message_port.cc:62: void
DidDetachInterstitialPage() override {
Hmm.  I suppose this will work, since the interstitial will go away if its RFH
dies.  (It's a bit unfortunate that we don't have a delete-time equivalent to
WCO::RenderFrameForInterstitialPageCreated, but that's probably unnecessary to
add here.)

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/extension_message_port.cc:63: if
(interstitial_frame_) {
nit: No braces for a one-line body.

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/extension_message_port.cc:78:
content::RenderFrameHost* interstitial_frame_;
This needs a comment saying that it's only present for interstitial main frames,
and that it's (presumably?) mutually exclusive the frames tracked normal tabs
(wherever those are tracked).

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/extension_message_port.cc:124: tab =
interstitial->web_contents();
This isn't obviously safe to me.  It depends on what you'll be doing with |tab|,
since in many respects it's not safe to treat the interstitial rfh like a frame
of the WebContents.  It's not in the FrameTree, and it won't get the same
notifications.

Is there a way to reason about why it's safe to use it below?  Can we add a
comment to that effect?

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/extension_message_port.cc:125: // This
is not expected to happen. When it happens, investigate why it
Let's elaborate on what "This" means here.  Are you talking about the case that
the interstitial doesn't have a WebContents?

(nit: Add blank line above this block.)

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/extension_message_port.cc:130: // Only
the main frame of an interstitial is supported.
Let's give a reason why subframes aren't supported (e.g., the sanity checker
doesn't allow it).  I don't think there are any interstitials with subframes,
but it's possible someone could hit this case and wonder about the reason.

https://codereview.chromium.org/1629013002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/1629013002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_delegate.h:122: virtual
InterstitialPage* GetAsInterstitialPage();
Yeah, I suppose we can use this pattern again.  I'll be glad to see it go away
when interstitials are refactored to be committed pages, but it makes sense for
the time being.

https://codereview.chromium.org/1629013002/diff/1/content/public/browser/inte...
File content/public/browser/interstitial_page.h (right):

https://codereview.chromium.org/1629013002/diff/1/content/public/browser/inte...
content/public/browser/interstitial_page.h:94: virtual WebContents*
web_contents() const = 0;
Style nit: We generally require CamelCase for virtual methods.

It'd be nice to avoid exposing this method if we can, since it becomes very easy
to forget that the interstitial's RFH and WebContents don't have a normal
relationship with each other (e.g., the RFH is not in the frame tree, and we
won't get the same WCO events for it).

I'm not sure if it's possible to avoid in your case or not, though.  If we do
keep it, let's put it above GetMainFrame and give some warning comments.

[robwu, 2016-01-25 22:54:41]

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/extension_message_port.cc:62: void
DidDetachInterstitialPage() override {
On 2016/01/25 21:47:38, Charlie Reis wrote:
> Hmm.  I suppose this will work, since the interstitial will go away if its RFH
> dies.  (It's a bit unfortunate that we don't have a delete-time equivalent to
> WCO::RenderFrameForInterstitialPageCreated, but that's probably unnecessary to
> add here.)

Acknowledged.

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/extension_message_port.cc:63: if
(interstitial_frame_) {
On 2016/01/25 21:47:38, Charlie Reis wrote:
> nit: No braces for a one-line body.

Done.

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/extension_message_port.cc:78:
content::RenderFrameHost* interstitial_frame_;
On 2016/01/25 21:47:38, Charlie Reis wrote:
> This needs a comment saying that it's only present for interstitial main
frames,
> and that it's (presumably?) mutually exclusive the frames tracked normal tabs
> (wherever those are tracked).

Done.

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/extension_message_port.cc:124: tab =
interstitial->web_contents();
On 2016/01/25 21:47:38, Charlie Reis wrote:
> This isn't obviously safe to me.  It depends on what you'll be doing with
|tab|,
> since in many respects it's not safe to treat the interstitial rfh like a
frame
> of the WebContents.  It's not in the FrameTree, and it won't get the same
> notifications.
> 
> Is there a way to reason about why it's safe to use it below?  Can we add a
> comment to that effect?

Done.

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/extension_message_port.cc:125: // This
is not expected to happen. When it happens, investigate why it
On 2016/01/25 21:47:38, Charlie Reis wrote:
> Let's elaborate on what "This" means here.  Are you talking about the case
that
> the interstitial doesn't have a WebContents?
> 
> (nit: Add blank line above this block.)

Done.

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/extension_message_port.cc:130: // Only
the main frame of an interstitial is supported.
On 2016/01/25 21:47:38, Charlie Reis wrote:
> Let's give a reason why subframes aren't supported (e.g., the sanity checker
> doesn't allow it).  I don't think there are any interstitials with subframes,
> but it's possible someone could hit this case and wonder about the reason.

Done.

https://codereview.chromium.org/1629013002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/1629013002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_delegate.h:122: virtual
InterstitialPage* GetAsInterstitialPage();
On 2016/01/25 21:47:38, Charlie Reis wrote:
> Yeah, I suppose we can use this pattern again.  I'll be glad to see it go away
> when interstitials are refactored to be committed pages, but it makes sense
for
> the time being.

Acknowledged.

https://codereview.chromium.org/1629013002/diff/1/content/public/browser/inte...
File content/public/browser/interstitial_page.h (right):

https://codereview.chromium.org/1629013002/diff/1/content/public/browser/inte...
content/public/browser/interstitial_page.h:94: virtual WebContents*
web_contents() const = 0;
On 2016/01/25 21:47:38, Charlie Reis wrote:
> Style nit: We generally require CamelCase for virtual methods.
> 
> It'd be nice to avoid exposing this method if we can, since it becomes very
easy
> to forget that the interstitial's RFH and WebContents don't have a normal
> relationship with each other (e.g., the RFH is not in the frame tree, and we
> won't get the same WCO events for it).
> 
> I'm not sure if it's possible to avoid in your case or not, though.  If we do
> keep it, let's put it above GetMainFrame and give some warning comments.

Done.

[Charlie Reis, 2016-01-26 01:14:30]

Thanks for the quick changes.  content/ LGTM with nits.

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/extension_message_port.cc:124: tab =
interstitial->web_contents();
On 2016/01/25 22:54:41, robwu wrote:
> On 2016/01/25 21:47:38, Charlie Reis wrote:
> > This isn't obviously safe to me.  It depends on what you'll be doing with
> |tab|,
> > since in many respects it's not safe to treat the interstitial rfh like a
> frame
> > of the WebContents.  It's not in the FrameTree, and it won't get the same
> > notifications.
> > 
> > Is there a way to reason about why it's safe to use it below?  Can we add a
> > comment to that effect?
> 
> Done.

I don't see any justification for this, but I think I understand it now.  Please
add the comment mentioned below.

https://codereview.chromium.org/1629013002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1629013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:137: // Only
the main frame of an interstitial is supported, because frames in
nit: Please move this comment above line 136, since it's about the conditional
and not the TrackInterstitialFrame call.

https://codereview.chromium.org/1629013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:144:
frame_tracker_->TrackInterstitialFrame(tab, rfh);
Please add a comment above this line:
It is safe to pass the interstitial's WebContents here because we only use it to
observe DidDetachInterstitialPage.

https://codereview.chromium.org/1629013002/diff/20001/content/public/browser/...
File content/public/browser/interstitial_page.h (right):

https://codereview.chromium.org/1629013002/diff/20001/content/public/browser/...
content/public/browser/interstitial_page.h:89: // notificaions (e.g.
RenderFrameDeleted). The only sensible use of this
nit: notifications

https://codereview.chromium.org/1629013002/diff/20001/content/public/browser/...
content/public/browser/interstitial_page.h:92: virtual WebContents*
GetUnderlyingWebContents() const = 0;
Underlying isn't a term we use elsewhere for interstitials and WebContents. 
Let's stick with GetWebContents() now that you have the nice warning comment.

[robwu, 2016-01-26 11:29:32]

Thanks for the quick review Charlie. I also moved a CHECK, but this should be
OK).

What remains is a review of the tests (chrome/test), which will be quite
straightforward.

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1629013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/extension_message_port.cc:124: tab =
interstitial->web_contents();
On 2016/01/26 01:14:30, Charlie Reis wrote:
> On 2016/01/25 22:54:41, robwu wrote:
> > On 2016/01/25 21:47:38, Charlie Reis wrote:
> > > This isn't obviously safe to me.  It depends on what you'll be doing with
> > |tab|,
> > > since in many respects it's not safe to treat the interstitial rfh like a
> > frame
> > > of the WebContents.  It's not in the FrameTree, and it won't get the same
> > > notifications.
> > > 
> > > Is there a way to reason about why it's safe to use it below?  Can we add
a
> > > comment to that effect?
> > 
> > Done.
> 
> I don't see any justification for this, but I think I understand it now. 
Please
> add the comment mentioned below.

Done.

https://codereview.chromium.org/1629013002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1629013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:137: // Only
the main frame of an interstitial is supported, because frames in
On 2016/01/26 01:14:30, Charlie Reis wrote:
> nit: Please move this comment above line 136, since it's about the conditional
> and not the TrackInterstitialFrame call.

Done.

https://codereview.chromium.org/1629013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:144:
frame_tracker_->TrackInterstitialFrame(tab, rfh);
On 2016/01/26 01:14:30, Charlie Reis wrote:
> Please add a comment above this line:
> It is safe to pass the interstitial's WebContents here because we only use it
to
> observe DidDetachInterstitialPage.

Done.

https://codereview.chromium.org/1629013002/diff/20001/content/public/browser/...
File content/public/browser/interstitial_page.h (right):

https://codereview.chromium.org/1629013002/diff/20001/content/public/browser/...
content/public/browser/interstitial_page.h:89: // notificaions (e.g.
RenderFrameDeleted). The only sensible use of this
On 2016/01/26 01:14:30, Charlie Reis wrote:
> nit: notifications

Done.

https://codereview.chromium.org/1629013002/diff/20001/content/public/browser/...
content/public/browser/interstitial_page.h:92: virtual WebContents*
GetUnderlyingWebContents() const = 0;
On 2016/01/26 01:14:30, Charlie Reis wrote:
> Underlying isn't a term we use elsewhere for interstitials and WebContents. 
> Let's stick with GetWebContents() now that you have the nice warning comment.

Done. (I called it GetUnderlyingWebContents because the class already had an
UnderlyingContentObserver, and it seemed like a good choice for a unique name.)

[Charlie Reis, 2016-01-26 23:58:27]

The test seems reasonable to me, though you'll want an extension reviewer for
that part rather than me.  Otherwise LGTM with nits.

https://codereview.chromium.org/1629013002/diff/20001/content/public/browser/...
File content/public/browser/interstitial_page.h (right):

https://codereview.chromium.org/1629013002/diff/20001/content/public/browser/...
content/public/browser/interstitial_page.h:92: virtual WebContents*
GetUnderlyingWebContents() const = 0;
On 2016/01/26 11:29:32, robwu wrote:
> On 2016/01/26 01:14:30, Charlie Reis wrote:
> > Underlying isn't a term we use elsewhere for interstitials and WebContents. 
> > Let's stick with GetWebContents() now that you have the nice warning
comment.
> 
> Done. (I called it GetUnderlyingWebContents because the class already had an
> UnderlyingContentObserver, and it seemed like a good choice for a unique
name.)

Ah, I was mistaken.  But yeah, that's hidden away in the implementation file, so
I like the simpler name here in the public API.

https://codereview.chromium.org/1629013002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1629013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:127:
content::InterstitialPage* interstitial = nullptr;
nit: Move declaration inside the if block, since it's not used outside.

https://codereview.chromium.org/1629013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:137: // then
WebContentsObserverSanityChecker triggers a CHECK when ut detects
nit: s/ut/it/

https://codereview.chromium.org/1629013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:140: // It is
safe to pass the interstitial's WebContents here because we use
nit: because we only use it to

https://codereview.chromium.org/1629013002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_messages_apitest.cc (right):

https://codereview.chromium.org/1629013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_messages_apitest.cc:171: // background.
nit: background page.

[robwu, 2016-01-27 23:30:06]

rob@robwu.nl changed reviewers:
+ rdevlin.cronin@chromium.org

[robwu, 2016-01-27 23:30:07]

Marijn/Devlin (whoever comes first): Can you check the tests and approve the
changes?

The tests are easy to review; if anything you'll probably find nits only.

This patch fixes a #1 crash on Canary.

https://codereview.chromium.org/1629013002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1629013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:127:
content::InterstitialPage* interstitial = nullptr;
On 2016/01/26 23:58:27, Charlie Reis wrote:
> nit: Move declaration inside the if block, since it's not used outside.

Done.

https://codereview.chromium.org/1629013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:137: // then
WebContentsObserverSanityChecker triggers a CHECK when ut detects
On 2016/01/26 23:58:27, Charlie Reis wrote:
> nit: s/ut/it/

Done.

https://codereview.chromium.org/1629013002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:140: // It is
safe to pass the interstitial's WebContents here because we use
On 2016/01/26 23:58:27, Charlie Reis wrote:
> nit: because we only use it to

Done.

[Devlin, 2016-01-27 23:46:15]

extensions lgtm

[robwu, 2016-01-27 23:54:28]

The CQ bit was checked by rob@robwu.nl

[robwu, 2016-01-27 23:54:29]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1629013002/#ps80001
(title: "Nits")

[commit-bot: I haz the power, 2016-01-27 23:57:10]

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

[commit-bot: I haz the power, 2016-01-28 00:14:14]

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

[commit-bot: I haz the power, 2016-01-28 00:14:15]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[robwu, 2016-01-28 00:19:13]

The CQ bit was checked by rob@robwu.nl

[robwu, 2016-01-28 00:19:14]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1629013002/#ps100001
(title: "Remove trailing space")

[commit-bot: I haz the power, 2016-01-28 00:21:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1629013002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1629013002/100001

[commit-bot: I haz the power, 2016-01-28 02:24:06]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-28 02:26:06]

Description was changed from

==========
Allow a main interstitial frame to act as an ExtensionMessagePort opener

BUG=575234,578828
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Allow a main interstitial frame to act as an ExtensionMessagePort opener

BUG=575234,578828
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/42079f3f4b4f940a77392ebda383ea943aeef511
Cr-Commit-Position: refs/heads/master@{#371969}
==========

[commit-bot: I haz the power, 2016-01-28 02:26:08]

Patchset 6 (id:??) landed as
https://crrev.com/42079f3f4b4f940a77392ebda383ea943aeef511
Cr-Commit-Position: refs/heads/master@{#371969}