Add the main frame routing ID to WebContentsDelegate, use it in BackgroundContents.

Add the main frame routing ID to WebContentsDelegate, use it in BackgroundContents.

BUG=438474
TEST=as in bug; no crash
TBR=thestig@chromium.org,atwilson@chromium.org

Committed: https://crrev.com/c2c29d784e3ba62afaee3f4cc16e4d72297ffbe6
Cr-Commit-Position: refs/heads/master@{#308008}

[Avi (use Gerrit), 2014-12-11 01:11:41]

avi@chromium.org changed reviewers:
+ asargent@chromium.org, creis@chromium.org, thestig@chromium.org

[Avi (use Gerrit), 2014-12-11 01:11:42]

asargent@chromium.org: Please review changes in extensions/
thestig@chromium.org: Please review changes in chrome/
creis@chromium.org: Everything else

[Avi (use Gerrit), 2014-12-11 01:23:06]

avi@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Avi (use Gerrit), 2014-12-11 01:23:06]

tedchoc@: please look at Android

[Ted C, 2014-12-11 01:25:00]

On 2014/12/11 01:23:06, Avi wrote:
> tedchoc@: please look at Android

android - lgtm

[Charlie Reis, 2014-12-11 17:48:18]

[+nasko to CC for main_frame_routing_id]

Wow, this is crazy.  How does anything work in a BackgroundContents if its main
RenderFrameHost can't receive IPCs?  Navigation IPCs go through there, so that
seems pretty fundamental.

At any rate, the fix looks good.  Are there any BackgroundContents tests that we
can update to ensure this doesn't regress?

https://codereview.chromium.org/790423002/diff/40001/chrome/browser/tab_conte...
File chrome/browser/tab_contents/background_contents.cc (right):

https://codereview.chromium.org/790423002/diff/40001/chrome/browser/tab_conte...
chrome/browser/tab_contents/background_contents.cc:38:
create_params.main_frame_routing_id = main_frame_routing_id;
What about other places that set CreateParams::routing_id?  Will they have a
similar bug?  (Looks like shell.cc might be the only other place from a quick
code search.)

[nasko, 2014-12-11 18:10:20]

On 2014/12/11 17:48:18, Charlie Reis wrote:
> [+nasko to CC for main_frame_routing_id]
> 
> Wow, this is crazy.  How does anything work in a BackgroundContents if its
main
> RenderFrameHost can't receive IPCs?  Navigation IPCs go through there, so that
> seems pretty fundamental.
> 
> At any rate, the fix looks good.  Are there any BackgroundContents tests that
we
> can update to ensure this doesn't regress?
> 
>
https://codereview.chromium.org/790423002/diff/40001/chrome/browser/tab_conte...
> File chrome/browser/tab_contents/background_contents.cc (right):
> 
>
https://codereview.chromium.org/790423002/diff/40001/chrome/browser/tab_conte...
> chrome/browser/tab_contents/background_contents.cc:38:
> create_params.main_frame_routing_id = main_frame_routing_id;
> What about other places that set CreateParams::routing_id?  Will they have a
> similar bug?  (Looks like shell.cc might be the only other place from a quick
> code search.)

I'd suggest we also add a DCHECK in WebContents::Create to ensure that we never
get a MSG_ROUTING_NONE for the main_frame_routing_id. I don't think we ever have
a case that would require such behavior.

[Avi (use Gerrit), 2014-12-11 19:28:00]

So...

1) Tests? What are those?

2) Nasko's comment about MSG_ROUTING_NONE for the main_frame_routing_id: That's
perfectly OK if the view's routing id is MSG_ROUTING_NONE too. In the situation
where the browser side is constructed first, both the view and frame hosts are
assigned routing ids and those ids are sent to the browser. Adding a check.

3) Charlie's comment about places that manually assign CreateParams::routing_id:
Yes, Shell::CreateNewWindow seems to be the only other place, but all calls pass
in MSG_ROUTING_NONE.

New rev.

[Avi (use Gerrit), 2014-12-11 19:28:00]

So...

1) Tests? What are those?

2) Nasko's comment about MSG_ROUTING_NONE for the main_frame_routing_id: That's
perfectly OK if the view's routing id is MSG_ROUTING_NONE too. In the situation
where the browser side is constructed first, both the view and frame hosts are
assigned routing ids and those ids are sent to the browser. Adding a check.

3) Charlie's comment about places that manually assign CreateParams::routing_id:
Yes, Shell::CreateNewWindow seems to be the only other place, but all calls pass
in MSG_ROUTING_NONE.

New rev.

[Charlie Reis, 2014-12-11 19:58:31]

creis@chromium.org changed reviewers:
+ atwilson@chromium.org

[Charlie Reis, 2014-12-11 19:58:32]

content/ LGTM, but I think a test for BackgroundContents should be added.  It's
crazy to me that the whole feature is broken and no one has noticed.  How is
Offline Gmail working?  Just in a fallback mode as if the background process
crashed?

@atwilson, is there a simple way to write a test for them?  Not being able to
process any RenderFrameHost IPCs is a serious regression that would presumably
prevent even navigation, and we don't have any way to tell that it has
regressed.

[Avi (use Gerrit), 2014-12-11 20:10:37]

TBRing: this fixes massive brokenness; the files that the TBR applies to are
where I needed to add a new parameter to functions that ignore it.

[Avi (use Gerrit), 2014-12-11 20:10:47]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-12-11 20:11:41]

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

[asargent_no_longer_on_chrome, 2014-12-11 20:29:28]

extensions/ lgtm

[Lei Zhang, 2014-12-11 22:42:41]

lgtm

[Avi (use Gerrit), 2014-12-11 22:55:57]

The CQ bit was unchecked by avi@chromium.org

[Avi (use Gerrit), 2014-12-11 22:56:04]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-12-11 23:10:51]

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

[Lei Zhang, 2014-12-11 23:29:41]

chrome/ lgtm

On Thu, Dec 11, 2014 at 12:29 PM,  <asargent@chromium.org> wrote:
> extensions/ lgtm
>
> https://codereview.chromium.org/790423002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Andrew T Wilson (Slow), 2014-12-12 00:01:54]

On Thu, Dec 11, 2014 at 11:58 AM, <creis@chromium.org> wrote:

> content/ LGTM, but I think a test for BackgroundContents should be added.
> It's
> crazy to me that the whole feature is broken and no one has noticed.  How
> is
> Offline Gmail working?  Just in a fallback mode as if the background
> process
> crashed?
>

./chrome/browser/extensions/app_background_page_apitest.cc contains
tests for background contents. It's possible that the test doesn't
check navigations, though. It also doesn't exercise a ton of
content-level functionality - it's just creating tabs and making sure
they exist and are not visible, and can execute javascript.


> @atwilson, is there a simple way to write a test for them?  Not being able
> to
> process any RenderFrameHost IPCs is a serious regression that would
> presumably
> prevent even navigation, and we don't have any way to tell that it has
> regressed.
>
>
> https://codereview.chromium.org/790423002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-12-12 00:24:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-12 00:24:45]

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