Fix for 'Simple Adblock' extension crashes

When deleting a WebContents, keep SessionStorageNamespaces used in the tab alive until we receive an acknowledgment from the renderer that the renderer side constructs have been cleaned up. Otherwise we can receive messages from still executing content referring to sessions that were prematurely deleted.

BUG=371304
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275383

[michaeln, 2014-06-02 20:13:39]

https://codereview.chromium.org/305103003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/305103003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:1052: // cast doesn't
work there.
i'll have to do something better about this, just want to see of tests pass with
this quick change

[michaeln, 2014-06-03 00:52:22]

ptal

[jsbell, 2014-06-03 16:56:39]

I don't see any problems, but there are parts of the code I don't feel confident
enough in to give it a l*g*t*m yet. I'll keep looking. Some initial
nits/questions.

https://codereview.chromium.org/305103003/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/305103003/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:188: // with mock hosts
as input in test cases.
I don't see this used in tests?

https://codereview.chromium.org/305103003/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:304: void
OnCloseACK(int old_route_id);
Nit: inconsistent capitalization (Ack vs. ACK)

https://codereview.chromium.org/305103003/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/305103003/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_view_host_delegate.h:15: #include
"content/public/browser/navigation_controller.h"
Is this include needed here?

https://codereview.chromium.org/305103003/diff/80001/content/common/view_mess...
File content/common/view_messages.h (right):

https://codereview.chromium.org/305103003/diff/80001/content/common/view_mess...
content/common/view_messages.h:1106: // Close  message.
Nit: extra space

https://codereview.chromium.org/305103003/diff/80001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/305103003/diff/80001/content/renderer/render_...
content/renderer/render_widget.cc:709: RenderThread::Get()->Send(new
ViewHostMsg_Close_ACK(routing_id_));
Since I'm new to the RenderViewImpl/RenderWidget code, can you explain why the
ACK is only sent out if OnClose on the second (or later) call?

[michaeln, 2014-06-03 18:58:09]

https://codereview.chromium.org/305103003/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/305103003/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:188: // with mock hosts
as input in test cases.
On 2014/06/03 16:56:39, jsbell wrote:
> I don't see this used in tests?

The RenderProcessHost used by the viewhost is a mock in some tests.

https://codereview.chromium.org/305103003/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:304: void
OnCloseACK(int old_route_id);
On 2014/06/03 16:56:39, jsbell wrote:
> Nit: inconsistent capitalization (Ack vs. ACK)

Done, going with ACK to match OnClosePageACK

https://codereview.chromium.org/305103003/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/305103003/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_view_host_delegate.h:15: #include
"content/public/browser/navigation_controller.h"
On 2014/06/03 16:56:39, jsbell wrote:
> Is this include needed here?

Yes, that's where the SessionStorageNamespaceMap typedef comes from.

https://codereview.chromium.org/305103003/diff/80001/content/common/view_mess...
File content/common/view_messages.h (right):

https://codereview.chromium.org/305103003/diff/80001/content/common/view_mess...
content/common/view_messages.h:1106: // Close  message.
On 2014/06/03 16:56:39, jsbell wrote:
> Nit: extra space

Done.

https://codereview.chromium.org/305103003/diff/80001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/305103003/diff/80001/content/renderer/render_...
content/renderer/render_widget.cc:709: RenderThread::Get()->Send(new
ViewHostMsg_Close_ACK(routing_id_));
On 2014/06/03 16:56:39, jsbell wrote:
> Since I'm new to the RenderViewImpl/RenderWidget code, can you explain why the
> ACK is only sent out if OnClose on the second (or later) call?

Its not, the ACK is normally sent in RenderView::Close. I've added it here too
because i noticed the odd early exit and wanted to send the ACK in more cases.
This still doesn't cover all cases because iff the View/Widget has been deleted
and the route removed, the Close msg will go unACK'd.

Since I'm new to the RenderViewImpl/RenderWidget too, I don't know the
circumstances in which closing_would be true or the view/widget would be deleted
prior to the Close msg arriving. Notice on line 718 the route id is removed,
given that line i don't see how the early exit makes sense. Also this is a
message handler that receives a routed message, so the test for routing_id_
against MSG_ROUTING_NONE looks odd to me.

Give those mysterious things, maybe i should not piggy pack an ack on the Close
msg, and instead define a new pair of ping/pong msgs to know when its safe to
release the namespaces. wdyt?

[michaeln1, 2014-06-03 19:44:44]

thats odd, the title and description got squashed somehow?

[michaeln, 2014-06-03 23:16:57]

james, can you please take a look

https://codereview.chromium.org/305103003/diff/80001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/305103003/diff/80001/content/renderer/render_...
content/renderer/render_widget.cc:709: RenderThread::Get()->Send(new
ViewHostMsg_Close_ACK(routing_id_));
On 2014/06/03 16:56:39, jsbell wrote:
> Since I'm new to the RenderViewImpl/RenderWidget code, can you explain why the
> ACK is only sent out if OnClose on the second (or later) call?

I've adjusted this so both callsites for sending the ack are in RenderViewImpl.

[jsbell, 2014-06-03 23:29:30]

lgtm with the latest change, but an OWNER review is necessary and a really good
idea. :)

https://codereview.chromium.org/305103003/diff/80001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/305103003/diff/80001/content/renderer/render_...
content/renderer/render_widget.cc:709: RenderThread::Get()->Send(new
ViewHostMsg_Close_ACK(routing_id_));
On 2014/06/03 18:58:09, michaeln wrote:
> Notice on line 718 the route id is removed,
> given that line i don't see how the early exit makes sense. Also this is a
> message handler that receives a routed message, so the test for routing_id_
> against MSG_ROUTING_NONE looks odd to me.

Ah yes. Definitely need someone more familiar with the code. But the latest
patch makes this concern mostly go away.

[jamesr, 2014-06-03 23:30:02]

I'm not a good reviewer for this change.

[michaeln, 2014-06-03 23:37:52]

On 2014/06/03 23:30:02, jamesr wrote:
> I'm not a good reviewer for this change.

can you suggest who is and should there be other owners listed?

[michaeln, 2014-06-03 23:57:09]

jochen, can you take a look please

[jochen (gone - plz use gerrit), 2014-06-04 08:16:24]

is it possible to write a test for this?

I guess the user data will still be destructed if the render processes dies
before it can send the close ack?

https://codereview.chromium.org/305103003/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/305103003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:359: public:
nit. add explicit ctor and virtual dtor

https://codereview.chromium.org/305103003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:371: };
nit. DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/305103003/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/305103003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:15: #include
"content/public/browser/navigation_controller.h"
why?

https://codereview.chromium.org/305103003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:16: #include
"content/public/browser/session_storage_namespace.h"
inside content, we should directly include the *impl.h headers

[michaeln, 2014-06-04 20:20:58]

On 2014/06/04 08:16:24, jochen wrote:
> is it possible to write a test for this?

probably a browsertest could test this, i'm not so familiar with these classes
or their test harnesses, i'll look into that

> I guess the user data will still be destructed if the render processes dies
> before it can send the close ack?

yes

>
https://codereview.chromium.org/305103003/diff/100001/content/browser/rendere...
> File content/browser/renderer_host/render_view_host_delegate.h (right):
> 
>
https://codereview.chromium.org/305103003/diff/100001/content/browser/rendere...
> content/browser/renderer_host/render_view_host_delegate.h:15: #include
> "content/public/browser/navigation_controller.h"
> why?

jsbell asked the same question, that's where typedef SessionStorageNamespaceMap
comes from

>
https://codereview.chromium.org/305103003/diff/100001/content/browser/rendere...
> content/browser/renderer_host/render_view_host_delegate.h:16: #include
> "content/public/browser/session_storage_namespace.h"
> inside content, we should directly include the *impl.h headers

this one might not be needed anymore? if it is i'll use the impl.h header

[michaeln, 2014-06-04 23:53:27]

added a test, ptal

https://codereview.chromium.org/305103003/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/305103003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:359: public:
On 2014/06/04 08:16:24, jochen wrote:
> nit. add explicit ctor and virtual dtor

Done.

https://codereview.chromium.org/305103003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:371: };
On 2014/06/04 08:16:24, jochen wrote:
> nit. DISALLOW_COPY_AND_ASSIGN()

Done.

https://codereview.chromium.org/305103003/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/305103003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:15: #include
"content/public/browser/navigation_controller.h"
> why?

SessionStorageNamespaceMap typedef is defined there

https://codereview.chromium.org/305103003/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:16: #include
"content/public/browser/session_storage_namespace.h"
On 2014/06/04 08:16:24, jochen wrote:
> inside content, we should directly include the *impl.h headers

Done.

[jochen (gone - plz use gerrit), 2014-06-05 07:03:02]

lgtm with comment addressed

https://codereview.chromium.org/305103003/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/305103003/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:20: #include
"content/public/browser/navigation_controller.h"
i think render process host including navigation controller is wrong. maybe the
typedef should be moved to session storage namespace.h?

[michaeln, 2014-06-05 18:44:19]

>
https://codereview.chromium.org/305103003/diff/120001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.h:20: #include
> "content/public/browser/navigation_controller.h"
> i think render process host including navigation controller is wrong. maybe
the
> typedef should be moved to session storage namespace.h?

sgtm

[michaeln, 2014-06-05 18:49:39]

also i have to add the .html file to test/data

[michaeln, 2014-06-05 19:49:34]

Hmmm... i think my test may have exposed some other bugs that can occur when
quickly deleting a webcontents with a navigation in its very early stages?

FATAL:env.cc(40)] Check failed: env.
FATAL:input_method_bridge.cc(130)] Check failed: host_.

I'm not going to be able to commit this test right now. Maybe its a bug in the
test, but i think latent bugs due to wc->Load; delete wc; seem pretty likely.

[michaeln, 2014-06-05 20:37:42]

> I'm not going to be able to commit this test right now. Maybe its a bug in the
> test, but i think latent bugs due to wc->Load; delete wc; seem pretty likely.

Oh... it may not be valid to delete shell directly, i'll try calling
shell->Close() instead. If thats true, a private or protected dtor would have
helped.

[michaeln, 2014-06-05 22:28:02]

@tsepez for view_messages.h review

[Tom Sepez, 2014-06-05 22:33:00]

Messages themselves LGTM, but what happens if the renderer never acks? Could a
compromised renderer prevent itself from being terminated?

[michaeln, 2014-06-05 22:35:46]

On 2014/06/05 22:33:00, Tom Sepez wrote:
> Messages themselves LGTM, but what happens if the renderer never acks? Could a
> compromised renderer prevent itself from being terminated?

nope, the ack doesn't affect a renderer processes lifetime in anyway

[Tom Sepez, 2014-06-05 22:36:39]

> nope, the ack doesn't affect a renderer processes lifetime in anyway
Ok. Good. Thanks.

[michaeln, 2014-06-05 23:36:09]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-06-05 23:38:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaeln@chromium.org/305103003/200001

[commit-bot: I haz the power, 2014-06-06 10:46:49]

Change committed as 275383