Fix event page process cleanup when running in --isolate-extensions mode.

Fix event page process cleanup when running in --isolate-extensions mode.

With --isolate-extensions, navigations can start in one RenderFrameHost
and complete in another. This was not possible to happen with extensions
related RenderFrameHosts in the past, so the code wasn't able to handle
it. This CL refactors the OnNetworkRequest(Started|Stopped) to use the
navigation request id as the stable identifier instead of the associated
RenderFrameHost.

BUG=612668
Committed: https://crrev.com/c25500369dd9a7f49d2b7d276556b2e22ba840c8
Cr-Commit-Position: refs/heads/master@{#397328}

[nasko, 2016-05-31 22:23:50]

nasko@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[nasko, 2016-05-31 22:23:50]

Hey Devlin,
Can you review this CL for me? It fixes the issue with event page extension
processes not going away with --isolate-extensions.
Thanks in advance!
Nasko

[Devlin, 2016-05-31 22:45:30]

https://codereview.chromium.org/2028873002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/lazy_background_page_apitest.cc (right):

https://codereview.chromium.org/2028873002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/lazy_background_page_apitest.cc:692:
EventProcessCleanup) {
This almost seems like a test that would be good to run in non-isolate
extensions mode, too, even if just as a sanity check.  Is there a reason to only
run it in --isolate-extensions, or could we make it take a param to run in both
modes?

https://codereview.chromium.org/2028873002/diff/20001/extensions/browser/proc...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/2028873002/diff/20001/extensions/browser/proc...
extensions/browser/process_manager.cc:576: make_pair(request_id,
host->extension()->id()));
unrelated - why doesn't make_pair need the std:: specifier?

https://codereview.chromium.org/2028873002/diff/20001/extensions/browser/proc...
extensions/browser/process_manager.cc:591:
host->OnNetworkRequestDone(request_id);
Are we going to have the same problem here with only conditionally reporting the
request as done?  It looks like the OnNetworkRequest(Started|Done) forwards to a
notification, which is used in
chrome/browser/chromeos/power/extension_event_observer.cc to do more
keep-aliving :(

[nasko, 2016-06-01 21:22:19]

https://codereview.chromium.org/2028873002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/lazy_background_page_apitest.cc (right):

https://codereview.chromium.org/2028873002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/lazy_background_page_apitest.cc:692:
EventProcessCleanup) {
On 2016/05/31 22:45:30, Devlin wrote:
> This almost seems like a test that would be good to run in non-isolate
> extensions mode, too, even if just as a sanity check.  Is there a reason to
only
> run it in --isolate-extensions, or could we make it take a param to run in
both
> modes?

There is already enough coverage for the regular case. I could look at making
this parametrized if you think it is worthwhile, but with the aim of making
--isolate-extensions be the default in this milestone or at most the next one,
this will become a default test anyway.

https://codereview.chromium.org/2028873002/diff/20001/extensions/browser/proc...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/2028873002/diff/20001/extensions/browser/proc...
extensions/browser/process_manager.cc:576: make_pair(request_id,
host->extension()->id()));
On 2016/05/31 22:45:30, Devlin wrote:
> unrelated - why doesn't make_pair need the std:: specifier?

Done.

https://codereview.chromium.org/2028873002/diff/20001/extensions/browser/proc...
extensions/browser/process_manager.cc:591:
host->OnNetworkRequestDone(request_id);
On 2016/05/31 22:45:30, Devlin wrote:
> Are we going to have the same problem here with only conditionally reporting
the
> request as done?  It looks like the OnNetworkRequest(Started|Done) forwards to
a
> notification, which is used in
> chrome/browser/chromeos/power/extension_event_observer.cc to do more
> keep-aliving :(

There were a bunch of problems with this version of the code. I've hopefully
fixed them and it should account for the extra keep-aliving in other places.

[Devlin, 2016-06-01 21:39:03]

lgtm

https://codereview.chromium.org/2028873002/diff/60001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/lazy_background_page/event_page_with_web_iframe/event_page.html
(right):

https://codereview.chromium.org/2028873002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/lazy_background_page/event_page_with_web_iframe/event_page.html:1:
<!DOCTYPE html>
nitty nit: I prefer not to shout:
<!doctype html>

https://codereview.chromium.org/2028873002/diff/60001/extensions/browser/proc...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/2028873002/diff/60001/extensions/browser/proc...
extensions/browser/process_manager.cc:589: ExtensionHost* host = result->second;
It might be worth noting that |host| can be invalid here (I know that's why
you're checking on line 592, but future us might not remember that it's unsafe
to access this on line 590 ;))

[nasko, 2016-06-01 23:17:51]

https://codereview.chromium.org/2028873002/diff/60001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/lazy_background_page/event_page_with_web_iframe/event_page.html
(right):

https://codereview.chromium.org/2028873002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/lazy_background_page/event_page_with_web_iframe/event_page.html:1:
<!DOCTYPE html>
On 2016/06/01 21:39:02, Devlin wrote:
> nitty nit: I prefer not to shout:
> <!doctype html>

I don't like it either, I just copy things over ;).

https://codereview.chromium.org/2028873002/diff/60001/extensions/browser/proc...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/2028873002/diff/60001/extensions/browser/proc...
extensions/browser/process_manager.cc:589: ExtensionHost* host = result->second;
On 2016/06/01 21:39:02, Devlin wrote:
> It might be worth noting that |host| can be invalid here (I know that's why
> you're checking on line 592, but future us might not remember that it's unsafe
> to access this on line 590 ;))

Done.

[nasko, 2016-06-01 23:18:05]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-06-01 23:18:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2028873002/#ps100001
(title: "Address review comments.")

[commit-bot: I haz the power, 2016-06-01 23:19:19]

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

[commit-bot: I haz the power, 2016-06-02 04:05:03]

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

[commit-bot: I haz the power, 2016-06-02 04:05:04]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[nasko, 2016-06-02 06:22:02]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2016-06-02 06:22:23]

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

[commit-bot: I haz the power, 2016-06-02 07:26:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-02 07:27:48]

Description was changed from

==========
Fix event page process cleanup when running in --isolate-extensions mode.

With --isolate-extensions, navigations can start in one RenderFrameHost
and complete in another. This was not possible to happen with extensions
related RenderFrameHosts in the past, so the code wasn't able to handle
it. This CL refactors the OnNetworkRequest(Started|Stopped) to use the
navigation request id as the stable identifier instead of the associated
RenderFrameHost.

BUG=612668
==========

to

==========
Fix event page process cleanup when running in --isolate-extensions mode.

With --isolate-extensions, navigations can start in one RenderFrameHost
and complete in another. This was not possible to happen with extensions
related RenderFrameHosts in the past, so the code wasn't able to handle
it. This CL refactors the OnNetworkRequest(Started|Stopped) to use the
navigation request id as the stable identifier instead of the associated
RenderFrameHost.

BUG=612668

Committed: https://crrev.com/c25500369dd9a7f49d2b7d276556b2e22ba840c8
Cr-Commit-Position: refs/heads/master@{#397328}
==========

[commit-bot: I haz the power, 2016-06-02 07:27:49]

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

[nasko, 2016-06-06 23:42:55]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2044643003/ by nasko@chromium.org.

The reason for reverting is: Speculative revert of this CL to check if it is the
root cause of https://crbug.com/616989..