Keep event page alive when there's some Pepper plugin on it.

Keep event page alive when there's some Pepper plugin on it.

BUG=472532
Committed: https://crrev.com/e70f5e1d286a200d420a996fcc2ff7fa7a2780a2
Cr-Commit-Position: refs/heads/master@{#331945}

[emaxx, 2015-05-04 09:35:02]

emaxx@chromium.org changed reviewers:
+ bartfab@chromium.org

[emaxx, 2015-05-04 09:36:40]

Bartosz, please have a look at this draft implementation.

Currently it violates the existing DEPS as it tries to include /extensions/ from
/content/. So we should find a way to rid of this explicit dependency.

[emaxx, 2015-05-07 13:30:07]

Updated the implementation: two new IPC messages between the renderer and the
browser are introduced, and they are used to notify the browser side about
creating and deleting a Pepper plugin instance in the page DOM.

[emaxx, 2015-05-11 13:41:25]

emaxx@chromium.org changed reviewers:
+ kalman@chromium.org, nasko@chromium.org, teravest@chromium.org

[emaxx, 2015-05-11 13:41:26]

kalman@chromium.org: Please review changes in extensions/ and
chrome/browser/extensions/.

nasko@chromium.org: Please review changes in content/.

teravest@chromium.org: Please review changes in ppapi/tests/.

[nasko, 2015-05-11 16:24:09]

The content/ code looks good in spirit, but should be refactored to not add new
code to RenderView and use RenderFrame instead.

https://codereview.chromium.org/1117023002/diff/110001/content/common/view_me...
File content/common/view_messages.h (right):

https://codereview.chromium.org/1117023002/diff/110001/content/common/view_me...
content/common/view_messages.h:1270:
IPC_MESSAGE_ROUTED0(ViewHostMsg_PepperInstanceCreated)
Please don't add any more IPCs for RenderView. It is a deprecated object and
shouldn't be getting new functionality.

https://codereview.chromium.org/1117023002/diff/110001/content/public/browser...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1117023002/diff/110001/content/public/browser...
content/public/browser/web_contents_observer.h:282: // attached/detached in the
page DOM.
The comment uses "attached/detached", but the name is Created/Deleted. Which one
is it? Can a pepper plugin instance be created and not attached to the DOM? If
yes, will this notification fire if that's the case?

https://codereview.chromium.org/1117023002/diff/110001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1117023002/diff/110001/content/renderer/rende...
content/renderer/render_view_impl.cc:1184: Send(new
ViewHostMsg_PepperInstanceCreated(GetRoutingID()));
This can be sent through the instance RenderFrame.

https://codereview.chromium.org/1117023002/diff/110001/content/renderer/rende...
content/renderer/render_view_impl.cc:1195: Send(new
ViewHostMsg_PepperInstanceDeleted(GetRoutingID()));
Same as above.

[not at google - send to devlin, 2015-05-11 17:53:48]

https://codereview.chromium.org/1117023002/diff/110001/chrome/browser/extensi...
File chrome/browser/extensions/lazy_background_page_apitest.cc (right):

https://codereview.chromium.org/1117023002/diff/110001/chrome/browser/extensi...
chrome/browser/extensions/lazy_background_page_apitest.cc:317: #if
!defined(DISABLE_NACL)
Is it better to ifdef out the entire test? That way you won't get misleading
tests passing if the bots don't run nacl. May as well just not run the test.

https://codereview.chromium.org/1117023002/diff/110001/chrome/browser/extensi...
chrome/browser/extensions/lazy_background_page_apitest.cc:333:
content::RunAllBlockingPoolTasksUntilIdle();
It's a pity, we're basically forcing this test to run > 10 seconds because
that's the timeout for event pages. Also, is RunAllBlockingPoolTasksUntilIdle
reliably going to wait for that, and/or not end up waiting for other things?
Surely other timers must go off.

For the former problem, perhaps at a scoped-override style of override for
g_event_page_idle_time_msec:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

I wonder if setting it to 0 would work... and for that matter, I wonder if
setting to anything > 0 would be flaky? (we'd probably want to make that work
anyway).

for the latter... I'm not sure what the most appropriate way to do this. Wait an
extra second? Ehk. There must be a better way.

https://codereview.chromium.org/1117023002/diff/110001/extensions/browser/ext...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1117023002/diff/110001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:161: if
(GetViewType(web_contents()) == VIEW_TYPE_EXTENSION_BACKGROUND_PAGE) {
I'm not sure I'd bother with this extra "is background page" check, just adds a
little bit of complexity.

[bartfab (slow), 2015-05-18 09:57:42]

https://codereview.chromium.org/1117023002/diff/110001/chrome/browser/extensi...
File chrome/browser/extensions/lazy_background_page_apitest.cc (right):

https://codereview.chromium.org/1117023002/diff/110001/chrome/browser/extensi...
chrome/browser/extensions/lazy_background_page_apitest.cc:320:
ASSERT_TRUE(PathService::Get(chrome::DIR_GEN_TEST_DATA, &extdir));
Nit: #include "testing/gtest/include/gtest/gtest.h"

https://codereview.chromium.org/1117023002/diff/110001/chrome/browser/extensi...
chrome/browser/extensions/lazy_background_page_apitest.cc:323:
ASSERT_TRUE(LoadExtension(base::FilePath(extdir)));
Why do you cast |extdir| to |base::FilePath| once more? It already has that
type.

https://codereview.chromium.org/1117023002/diff/110001/chrome/browser/extensi...
chrome/browser/extensions/lazy_background_page_apitest.cc:333:
content::RunAllBlockingPoolTasksUntilIdle();
On 2015/05/11 17:53:48, kalman wrote:
> It's a pity, we're basically forcing this test to run > 10 seconds because
> that's the timeout for event pages. Also, is RunAllBlockingPoolTasksUntilIdle
> reliably going to wait for that, and/or not end up waiting for other things?
> Surely other timers must go off.
> 
> For the former problem, perhaps at a scoped-override style of override for
> g_event_page_idle_time_msec:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> 
> I wonder if setting it to 0 would work... and for that matter, I wonder if
> setting to anything > 0 would be flaky? (we'd probably want to make that work
> anyway).
> 
> for the latter... I'm not sure what the most appropriate way to do this. Wait
an
> extra second? Ehk. There must be a better way.

I would be worried not just about early returns but also about the opposite -
what if something else posts an unrelated task to the blocking pool, keeping it
busy for a long time, causing this test to time out and flake?

+1 to overriding the idle time or alternatively, manually triggering an idle
event from here.

https://codereview.chromium.org/1117023002/diff/110001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1117023002/diff/110001/content/renderer/rende...
content/renderer/render_view_impl.cc:207: #include
"content/renderer/pepper/plugin_module.h"
Nit: Where are you using these new includes?

https://codereview.chromium.org/1117023002/diff/110001/extensions/browser/ext...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1117023002/diff/110001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:161: if
(GetViewType(web_contents()) == VIEW_TYPE_EXTENSION_BACKGROUND_PAGE) {
On 2015/05/11 17:53:48, kalman wrote:
> I'm not sure I'd bother with this extra "is background page" check, just adds
a
> little bit of complexity.

If we leave out this check, we will be keeping background pages alive even if
there is a NaCl module in the main/foreground page. That seems wasteful?

https://codereview.chromium.org/1117023002/diff/110001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:162: ProcessManager*
process_manager = ProcessManager::Get(browser_context_);
Nit 1: const pointer.
Nit 2: If you move this inside the conditional that follows, we will avoid
needlessly getting a |process_manager| in the case that |extension| turns out to
be a nullptr.

https://codereview.chromium.org/1117023002/diff/110001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:163: const Extension*
extension =
Nit 1: const pointer.
Nit 2: #include "extensions/common/extension.h"

https://codereview.chromium.org/1117023002/diff/110001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:172: ProcessManager*
process_manager = ProcessManager::Get(browser_context_);
Nit 1: const pointer.
Nit 2: As above, better move this inside the conditional.

https://codereview.chromium.org/1117023002/diff/110001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:173: const Extension*
extension =
Nit: const pointer.

https://codereview.chromium.org/1117023002/diff/110001/ppapi/tests/extensions...
File ppapi/tests/extensions/load_unload/background.js (right):

https://codereview.chromium.org/1117023002/diff/110001/ppapi/tests/extensions...
ppapi/tests/extensions/load_unload/background.js:24: }
Nit: Reset |nacl| back to |null|.

https://codereview.chromium.org/1117023002/diff/110001/ppapi/tests/extensions...
File ppapi/tests/extensions/load_unload/load_unload.cc (right):

https://codereview.chromium.org/1117023002/diff/110001/ppapi/tests/extensions...
ppapi/tests/extensions/load_unload/load_unload.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
Nit: s/(c) //

[emaxx, 2015-05-18 15:06:30]

Updated implementation: use RenderFrame where possible, make the new test
faster.

https://codereview.chromium.org/1117023002/diff/110001/chrome/browser/extensi...
File chrome/browser/extensions/lazy_background_page_apitest.cc (right):

https://codereview.chromium.org/1117023002/diff/110001/chrome/browser/extensi...
chrome/browser/extensions/lazy_background_page_apitest.cc:317: #if
!defined(DISABLE_NACL)
On 2015/05/11 17:53:48, kalman wrote:
> Is it better to ifdef out the entire test? That way you won't get misleading
> tests passing if the bots don't run nacl. May as well just not run the test.

Done.

https://codereview.chromium.org/1117023002/diff/110001/chrome/browser/extensi...
chrome/browser/extensions/lazy_background_page_apitest.cc:320:
ASSERT_TRUE(PathService::Get(chrome::DIR_GEN_TEST_DATA, &extdir));
On 2015/05/18 09:57:41, bartfab wrote:
> Nit: #include "testing/gtest/include/gtest/gtest.h"

Done.

https://codereview.chromium.org/1117023002/diff/110001/chrome/browser/extensi...
chrome/browser/extensions/lazy_background_page_apitest.cc:323:
ASSERT_TRUE(LoadExtension(base::FilePath(extdir)));
On 2015/05/18 09:57:41, bartfab wrote:
> Why do you cast |extdir| to |base::FilePath| once more? It already has that
> type.
Done (it was left from some old version).

https://codereview.chromium.org/1117023002/diff/110001/chrome/browser/extensi...
chrome/browser/extensions/lazy_background_page_apitest.cc:333:
content::RunAllBlockingPoolTasksUntilIdle();
On 2015/05/11 17:53:48, kalman wrote:
> It's a pity, we're basically forcing this test to run > 10 seconds because
> that's the timeout for event pages. Also, is RunAllBlockingPoolTasksUntilIdle
> reliably going to wait for that, and/or not end up waiting for other things?
> Surely other timers must go off.
> 
> For the former problem, perhaps at a scoped-override style of override for
> g_event_page_idle_time_msec:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> 
> I wonder if setting it to 0 would work... and for that matter, I wonder if
> setting to anything > 0 would be flaky? (we'd probably want to make that work
> anyway).
> 
> for the latter... I'm not sure what the most appropriate way to do this. Wait
an
> extra second? Ehk. There must be a better way.

Actually the test class LazyBackgroundPageApiTest is already changing the delay
time from the default 10 seconds to 1 millisecond. But it was doing this inside
SetUpOnMainThread(), which turns out to be too late. Now I've fixed it (using
SetUpInProcessBrowserTestFixture() instead).

Now this test works about 1.5 - 2 seconds, the same as any other test in that
file (and most of the other tests have the similar structure, BTW).

https://codereview.chromium.org/1117023002/diff/110001/chrome/browser/extensi...
chrome/browser/extensions/lazy_background_page_apitest.cc:333:
content::RunAllBlockingPoolTasksUntilIdle();
On 2015/05/18 09:57:41, bartfab wrote:
> On 2015/05/11 17:53:48, kalman wrote:
> > It's a pity, we're basically forcing this test to run > 10 seconds because
> > that's the timeout for event pages. Also, is
RunAllBlockingPoolTasksUntilIdle
> > reliably going to wait for that, and/or not end up waiting for other things?
> > Surely other timers must go off.
> > 
> > For the former problem, perhaps at a scoped-override style of override for
> > g_event_page_idle_time_msec:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> > 
> > I wonder if setting it to 0 would work... and for that matter, I wonder if
> > setting to anything > 0 would be flaky? (we'd probably want to make that
work
> > anyway).
> > 
> > for the latter... I'm not sure what the most appropriate way to do this.
Wait
> an
> > extra second? Ehk. There must be a better way.
> 
> I would be worried not just about early returns but also about the opposite -
> what if something else posts an unrelated task to the blocking pool, keeping
it
> busy for a long time, causing this test to time out and flake?
> 
> +1 to overriding the idle time or alternatively, manually triggering an idle
> event from here.

Done (see the answer above).

https://codereview.chromium.org/1117023002/diff/110001/content/common/view_me...
File content/common/view_messages.h (right):

https://codereview.chromium.org/1117023002/diff/110001/content/common/view_me...
content/common/view_messages.h:1270:
IPC_MESSAGE_ROUTED0(ViewHostMsg_PepperInstanceCreated)
On 2015/05/11 16:24:08, nasko wrote:
> Please don't add any more IPCs for RenderView. It is a deprecated object and
> shouldn't be getting new functionality.

Done.

https://codereview.chromium.org/1117023002/diff/110001/content/public/browser...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1117023002/diff/110001/content/public/browser...
content/public/browser/web_contents_observer.h:282: // attached/detached in the
page DOM.
On 2015/05/11 16:24:09, nasko wrote:
> The comment uses "attached/detached", but the name is Created/Deleted. Which
one
> is it? Can a pepper plugin instance be created and not attached to the DOM? If
> yes, will this notification fire if that's the case?

The notifications are only about Pepper instances appearing/disappearing in the
page DOM tree (though I don't think it's possible to create a Pepper instance
separate from any DOM tree).
Anyway, I've changed the wording to use "created"/"deleted" everywhere.

https://codereview.chromium.org/1117023002/diff/110001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1117023002/diff/110001/content/renderer/rende...
content/renderer/render_view_impl.cc:207: #include
"content/renderer/pepper/plugin_module.h"
On 2015/05/18 09:57:41, bartfab wrote:
> Nit: Where are you using these new includes?

Done.

https://codereview.chromium.org/1117023002/diff/110001/content/renderer/rende...
content/renderer/render_view_impl.cc:1184: Send(new
ViewHostMsg_PepperInstanceCreated(GetRoutingID()));
On 2015/05/11 16:24:09, nasko wrote:
> This can be sent through the instance RenderFrame.

I'm not sure what did you mean. Now I've changed the code here to send the
message through GetMainRenderFrame() - is that what you wanted?

https://codereview.chromium.org/1117023002/diff/110001/content/renderer/rende...
content/renderer/render_view_impl.cc:1195: Send(new
ViewHostMsg_PepperInstanceDeleted(GetRoutingID()));
On 2015/05/11 16:24:09, nasko wrote:
> Same as above.

Done.

https://codereview.chromium.org/1117023002/diff/110001/extensions/browser/ext...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1117023002/diff/110001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:161: if
(GetViewType(web_contents()) == VIEW_TYPE_EXTENSION_BACKGROUND_PAGE) {
On 2015/05/11 17:53:48, kalman wrote:
> I'm not sure I'd bother with this extra "is background page" check, just adds
a
> little bit of complexity.
OK, removed this check as it didn't make much difference.

https://codereview.chromium.org/1117023002/diff/110001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:161: if
(GetViewType(web_contents()) == VIEW_TYPE_EXTENSION_BACKGROUND_PAGE) {
On 2015/05/18 09:57:41, bartfab wrote:
> On 2015/05/11 17:53:48, kalman wrote:
> > I'm not sure I'd bother with this extra "is background page" check, just
adds
> a
> > little bit of complexity.
> 
> If we leave out this check, we will be keeping background pages alive even if
> there is a NaCl module in the main/foreground page. That seems wasteful?

I've already talked to Ben about that. The point is that when there're some
main/foreground pages, the keepalive count is non-zero anyway, so in fact we
won't change anything.

https://codereview.chromium.org/1117023002/diff/110001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:162: ProcessManager*
process_manager = ProcessManager::Get(browser_context_);
On 2015/05/18 09:57:41, bartfab wrote:
> Nit 1: const pointer.
> Nit 2: If you move this inside the conditional that follows, we will avoid
> needlessly getting a |process_manager| in the case that |extension| turns out
to
> be a nullptr.

1: Done.
2: Didn't get your point: the ProcessManager is required in the next line
already.

https://codereview.chromium.org/1117023002/diff/110001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:163: const Extension*
extension =
On 2015/05/18 09:57:41, bartfab wrote:
> Nit 1: const pointer.
> Nit 2: #include "extensions/common/extension.h"

Done.

https://codereview.chromium.org/1117023002/diff/110001/ppapi/tests/extensions...
File ppapi/tests/extensions/load_unload/background.js (right):

https://codereview.chromium.org/1117023002/diff/110001/ppapi/tests/extensions...
ppapi/tests/extensions/load_unload/background.js:24: }
On 2015/05/18 09:57:41, bartfab wrote:
> Nit: Reset |nacl| back to |null|.

Done.

https://codereview.chromium.org/1117023002/diff/110001/ppapi/tests/extensions...
File ppapi/tests/extensions/load_unload/load_unload.cc (right):

https://codereview.chromium.org/1117023002/diff/110001/ppapi/tests/extensions...
ppapi/tests/extensions/load_unload/load_unload.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/05/18 09:57:41, bartfab wrote:
> Nit: s/(c) //

Done.

[bartfab (slow), 2015-05-19 10:34:59]

lgtm

https://codereview.chromium.org/1117023002/diff/110001/extensions/browser/ext...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1117023002/diff/110001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:162: ProcessManager*
process_manager = ProcessManager::Get(browser_context_);
On 2015/05/18 15:06:30, emaxx wrote:
> On 2015/05/18 09:57:41, bartfab wrote:
> > Nit 1: const pointer.
> > Nit 2: If you move this inside the conditional that follows, we will avoid
> > needlessly getting a |process_manager| in the case that |extension| turns
out
> to
> > be a nullptr.
> 
> 1: Done.
> 2: Didn't get your point: the ProcessManager is required in the next line
> already.

Nevermind :).

[nasko, 2015-05-19 14:11:45]

https://codereview.chromium.org/1117023002/diff/110001/content/public/browser...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1117023002/diff/110001/content/public/browser...
content/public/browser/web_contents_observer.h:282: // attached/detached in the
page DOM.
On 2015/05/18 15:06:29, emaxx wrote:
> On 2015/05/11 16:24:09, nasko wrote:
> > The comment uses "attached/detached", but the name is Created/Deleted. Which
> one
> > is it? Can a pepper plugin instance be created and not attached to the DOM?
If
> > yes, will this notification fire if that's the case?
> 
> The notifications are only about Pepper instances appearing/disappearing in
the
> page DOM tree (though I don't think it's possible to create a Pepper instance
> separate from any DOM tree).
> Anyway, I've changed the wording to use "created"/"deleted" everywhere.

If this notification is about plugin being added to the DOM, while has already
been created, then it seems to me that it is better to rename the methods to
reflect reality instead of bring confusion.

https://codereview.chromium.org/1117023002/diff/110001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1117023002/diff/110001/content/renderer/rende...
content/renderer/render_view_impl.cc:1184: Send(new
ViewHostMsg_PepperInstanceCreated(GetRoutingID()));
On 2015/05/18 15:06:30, emaxx wrote:
> On 2015/05/11 16:24:09, nasko wrote:
> > This can be sent through the instance RenderFrame.
> 
> I'm not sure what did you mean. Now I've changed the code here to send the
> message through GetMainRenderFrame() - is that what you wanted?

This seems slightly incorrect. The PepperPluginInstanceImpl is associated with a
RenderFrame instance, which is accessible through render_frame(). See
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p....
This should be the frame used to send the message, not the main frame.

[emaxx, 2015-05-19 17:33:40]

Addressed Nasko's comments.

https://codereview.chromium.org/1117023002/diff/110001/content/public/browser...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1117023002/diff/110001/content/public/browser...
content/public/browser/web_contents_observer.h:282: // attached/detached in the
page DOM.
On 2015/05/19 14:11:45, nasko wrote:
> On 2015/05/18 15:06:29, emaxx wrote:
> > On 2015/05/11 16:24:09, nasko wrote:
> > > The comment uses "attached/detached", but the name is Created/Deleted.
Which
> > one
> > > is it? Can a pepper plugin instance be created and not attached to the
DOM?
> If
> > > yes, will this notification fire if that's the case?
> > 
> > The notifications are only about Pepper instances appearing/disappearing in
> the
> > page DOM tree (though I don't think it's possible to create a Pepper
instance
> > separate from any DOM tree).
> > Anyway, I've changed the wording to use "created"/"deleted" everywhere.
> 
> If this notification is about plugin being added to the DOM, while has already
> been created, then it seems to me that it is better to rename the methods to
> reflect reality instead of bring confusion.

As far as I understand, no real loading is performed before the <embed> is added
to the DOM (Ben, please correct me if that's not true). So I don't think that
brings any confusion.

Also, the same naming style is already used in the existing members of the
RenderViewImpl class:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

https://codereview.chromium.org/1117023002/diff/110001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1117023002/diff/110001/content/renderer/rende...
content/renderer/render_view_impl.cc:1184: Send(new
ViewHostMsg_PepperInstanceCreated(GetRoutingID()));
On 2015/05/19 14:11:45, nasko wrote:
> On 2015/05/18 15:06:30, emaxx wrote:
> > On 2015/05/11 16:24:09, nasko wrote:
> > > This can be sent through the instance RenderFrame.
> > 
> > I'm not sure what did you mean. Now I've changed the code here to send the
> > message through GetMainRenderFrame() - is that what you wanted?
> 
> This seems slightly incorrect. The PepperPluginInstanceImpl is associated with
a
> RenderFrame instance, which is accessible through render_frame(). See
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p....
> This should be the frame used to send the message, not the main frame.

Done.

[nasko, 2015-05-19 17:49:34]

https://codereview.chromium.org/1117023002/diff/170001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1117023002/diff/170001/content/renderer/rende...
content/renderer/render_view_impl.cc:1184: if (render_frame)
RenderFrame should never be null, so there is no reason to check the pointer for
validity. A crash on the call to Send would indicate a bug and we will want to
know about it.

[emaxx, 2015-05-19 18:14:59]

Change according to Nasko's comment.

https://codereview.chromium.org/1117023002/diff/170001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1117023002/diff/170001/content/renderer/rende...
content/renderer/render_view_impl.cc:1184: if (render_frame)
On 2015/05/19 17:49:34, nasko wrote:
> RenderFrame should never be null, so there is no reason to check the pointer
for
> validity. A crash on the call to Send would indicate a bug and we will want to
> know about it.

You are right.
I've now placed a DCHECK here - to reassure and to show the intention.

[nasko, 2015-05-19 18:51:51]

LGTM with nit.

https://codereview.chromium.org/1117023002/diff/170001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1117023002/diff/170001/content/renderer/rende...
content/renderer/render_view_impl.cc:1184: if (render_frame)
On 2015/05/19 18:14:59, emaxx wrote:
> On 2015/05/19 17:49:34, nasko wrote:
> > RenderFrame should never be null, so there is no reason to check the pointer
> for
> > validity. A crash on the call to Send would indicate a bug and we will want
to
> > know about it.
> 
> You are right.
> I've now placed a DCHECK here - to reassure and to show the intention.

In general, we don't add DCHECK when the line after is going to crash anyway.
The nullptr crash is equivalent to the DCHECK, so let's remove those.

[emaxx, 2015-05-20 20:11:25]

emaxx@chromium.org changed reviewers:
+ raymes@chromium.org
- teravest@chromium.org

[emaxx, 2015-05-20 20:11:26]

Hi Raymes, please take a look at the changes under ppapi/tests/ directory.

(teravest@ turned out to have transferred off of Chrome.)

[raymes, 2015-05-22 02:32:14]

On 2015/05/20 20:11:26, emaxx wrote:
> Hi Raymes, please take a look at the changes under ppapi/tests/ directory.
> 
> (teravest@ turned out to have transferred off of Chrome.)

rubberstamp lgtm ppapi/

[emaxx, 2015-05-26 09:23:04]

On 2015/05/19 18:51:51, nasko wrote:
> In general, we don't add DCHECK when the line after is going to crash anyway.
> The nullptr crash is equivalent to the DCHECK, so let's remove those.

I agree, the crash here seems inevitable.
But, wouldn't the DCHECK be useful here as a documentation element? If we decide
to remove the DCHECK and if in the future a crash happens here, it won't be so
obvious to the reader of the code: it may look like we just forgot to add "if
(!= NULL)" condition - like the one that's 15 lines below.

[nasko, 2015-05-26 16:50:04]

On 2015/05/26 09:23:04, emaxx wrote:
> On 2015/05/19 18:51:51, nasko wrote:
> > In general, we don't add DCHECK when the line after is going to crash
anyway.
> > The nullptr crash is equivalent to the DCHECK, so let's remove those.
> 
> I agree, the crash here seems inevitable.
> But, wouldn't the DCHECK be useful here as a documentation element? If we
decide
> to remove the DCHECK and if in the future a crash happens here, it won't be so
> obvious to the reader of the code: it may look like we just forgot to add "if
> (!= NULL)" condition - like the one that's 15 lines below.

The documentation is part of the Pepper instance being a RenderFrameObserver. If
the object is alive, it must have a valid RenderFrame pointer. 
Can you point out the specific code you have in mind (15 lines below which one)?

[not at google - send to devlin, 2015-05-26 21:18:42]

lgtm

https://codereview.chromium.org/1117023002/diff/190001/extensions/browser/ext...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1117023002/diff/190001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:162: const Extension*
const extension =
Kinda overboard-ing the "const" here IMO, but whatever you like.

[emaxx, 2015-05-29 10:23:28]

On 2015/05/26 16:50:04, nasko wrote:
> On 2015/05/26 09:23:04, emaxx wrote:
> > On 2015/05/19 18:51:51, nasko wrote:
> > > In general, we don't add DCHECK when the line after is going to crash
> anyway.
> > > The nullptr crash is equivalent to the DCHECK, so let's remove those.
> > 
> > I agree, the crash here seems inevitable.
> > But, wouldn't the DCHECK be useful here as a documentation element? If we
> decide
> > to remove the DCHECK and if in the future a crash happens here, it won't be
so
> > obvious to the reader of the code: it may look like we just forgot to add
"if
> > (!= NULL)" condition - like the one that's 15 lines below.
> 
> The documentation is part of the Pepper instance being a RenderFrameObserver.
If
> the object is alive, it must have a valid RenderFrame pointer. 
> Can you point out the specific code you have in mind (15 lines below which
one)?

I meant the "if (render_frame)" condition in PepperInstanceDeleted() method.
Anyway, as you insist on that this dcheck is pointless, I've removed it now.

[emaxx, 2015-05-29 10:23:39]

The CQ bit was checked by emaxx@chromium.org

[emaxx, 2015-05-29 10:23:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, raymes@chromium.org, nasko@chromium.org,
kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1117023002/#ps210001
(title: "Remove DCHECK.")

[commit-bot: I haz the power, 2015-05-29 10:23:47]

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

[commit-bot: I haz the power, 2015-05-29 11:26:16]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2015-05-29 11:26:59]

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

[nasko, 2015-05-29 14:01:14]

On 2015/05/29 10:23:28, emaxx wrote:
> On 2015/05/26 16:50:04, nasko wrote:
> > On 2015/05/26 09:23:04, emaxx wrote:
> > > On 2015/05/19 18:51:51, nasko wrote:
> > > > In general, we don't add DCHECK when the line after is going to crash
> > anyway.
> > > > The nullptr crash is equivalent to the DCHECK, so let's remove those.
> > > 
> > > I agree, the crash here seems inevitable.
> > > But, wouldn't the DCHECK be useful here as a documentation element? If we
> > decide
> > > to remove the DCHECK and if in the future a crash happens here, it won't
be
> so
> > > obvious to the reader of the code: it may look like we just forgot to add
> "if
> > > (!= NULL)" condition - like the one that's 15 lines below.
> > 
> > The documentation is part of the Pepper instance being a
RenderFrameObserver.
> If
> > the object is alive, it must have a valid RenderFrame pointer. 
> > Can you point out the specific code you have in mind (15 lines below which
> one)?
> 
> I meant the "if (render_frame)" condition in PepperInstanceDeleted() method.
> Anyway, as you insist on that this dcheck is pointless, I've removed it now.

Well, my comment applied to both, since they are identical usage. I didn't flag
them separately, as my assumption is once an issue is pointed out, one can
search for all of its occurrences. Please fix it in a follow up CL, since you've
landed this one already.

[emaxx, 2015-06-02 02:06:19]

On 2015/05/29 14:01:14, nasko wrote:
> On 2015/05/29 10:23:28, emaxx wrote:
> > On 2015/05/26 16:50:04, nasko wrote:
> > > On 2015/05/26 09:23:04, emaxx wrote:
> > > > On 2015/05/19 18:51:51, nasko wrote:
> > > > > In general, we don't add DCHECK when the line after is going to crash
> > > anyway.
> > > > > The nullptr crash is equivalent to the DCHECK, so let's remove those.
> > > > 
> > > > I agree, the crash here seems inevitable.
> > > > But, wouldn't the DCHECK be useful here as a documentation element? If
we
> > > decide
> > > > to remove the DCHECK and if in the future a crash happens here, it won't
> be
> > so
> > > > obvious to the reader of the code: it may look like we just forgot to
add
> > "if
> > > > (!= NULL)" condition - like the one that's 15 lines below.
> > > 
> > > The documentation is part of the Pepper instance being a
> RenderFrameObserver.
> > If
> > > the object is alive, it must have a valid RenderFrame pointer. 
> > > Can you point out the specific code you have in mind (15 lines below which
> > one)?
> > 
> > I meant the "if (render_frame)" condition in PepperInstanceDeleted() method.
> > Anyway, as you insist on that this dcheck is pointless, I've removed it now.
> 
> Well, my comment applied to both, since they are identical usage. I didn't
flag
> them separately, as my assumption is once an issue is pointed out, one can
> search for all of its occurrences. Please fix it in a follow up CL, since
you've
> landed this one already.

Here it is explicitly stated that PepperPluginInstanceImpl and RenderViewImpl
may outlive RenderFrameImpl:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
So when PepperInstanceDeleted() is called, there is no guarantee that
RenderFrameImpl still exists. Am I missing something?

[nasko, 2015-06-02 23:53:28]

On 2015/06/02 02:06:19, emaxx wrote:
> On 2015/05/29 14:01:14, nasko wrote:
> > On 2015/05/29 10:23:28, emaxx wrote:
> > > On 2015/05/26 16:50:04, nasko wrote:
> > > > On 2015/05/26 09:23:04, emaxx wrote:
> > > > > On 2015/05/19 18:51:51, nasko wrote:
> > > > > > In general, we don't add DCHECK when the line after is going to
crash
> > > > anyway.
> > > > > > The nullptr crash is equivalent to the DCHECK, so let's remove
those.
> > > > > 
> > > > > I agree, the crash here seems inevitable.
> > > > > But, wouldn't the DCHECK be useful here as a documentation element? If
> we
> > > > decide
> > > > > to remove the DCHECK and if in the future a crash happens here, it
won't
> > be
> > > so
> > > > > obvious to the reader of the code: it may look like we just forgot to
> add
> > > "if
> > > > > (!= NULL)" condition - like the one that's 15 lines below.
> > > > 
> > > > The documentation is part of the Pepper instance being a
> > RenderFrameObserver.
> > > If
> > > > the object is alive, it must have a valid RenderFrame pointer. 
> > > > Can you point out the specific code you have in mind (15 lines below
which
> > > one)?
> > > 
> > > I meant the "if (render_frame)" condition in PepperInstanceDeleted()
method.
> > > Anyway, as you insist on that this dcheck is pointless, I've removed it
now.
> > 
> > Well, my comment applied to both, since they are identical usage. I didn't
> flag
> > them separately, as my assumption is once an issue is pointed out, one can
> > search for all of its occurrences. Please fix it in a follow up CL, since
> you've
> > landed this one already.
> 
> Here it is explicitly stated that PepperPluginInstanceImpl and RenderViewImpl
> may outlive RenderFrameImpl:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
> So when PepperInstanceDeleted() is called, there is no guarantee that
> RenderFrameImpl still exists. Am I missing something?

I wasn't aware of this ugly artifact that PepperPluginInstance lifetime is not
quite tied to RenderFrameImpl. It looks like your code is indeed right in
RVH::PepperInstanceDeleted. We need to clean up this area of the code to ensure
that Pepper is fully moved over to use RenderFrameImpl instead of
RenderViewImpl.