Simplify WebNavigationApi by using RenderFrameHost internally.

Simplify WebNavigationApi by using RenderFrameHost internally.

WebNavigationApi formerly passed around a lot of redundant state, but
now it just uses RenderFrameHost. Code that needs to know the routing
ID, parent, etc can just derive it from the RenderFrameHost.

Since WebNavigationApi is storing raw pointers to RenderFrameHost, it
also observers RFH deletions and removes them as needed. This fixes
some use-after-frees when other observers are incorrectly causing the
RenderFrameHost to be destroyed.

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

[dcheng, 2014-07-12 00:28:58]

Note this probably collides/overlaps in functionality with
https://codereview.chromium.org/380213003. Please let me know how you'd like me
to proceed WRT your patch--I suspect that my patch may also fix
http://crbug.com/392718, but I'm not 100% sure.

[jochen (gone - plz use gerrit), 2014-07-14 09:38:36]

lgtm

please at least add all the NOTREACHED() for cases where we pass a render frame
host to the frame navigation state class which it doesn't know about. Or add
CHECKs().

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc
(right):

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:82:
frame_host_state_map_.erase(frame_host);
can we add at least DCHECK()s (or even CHECKs()) that the frame_host is known?

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:82:
frame_host_state_map_.erase(frame_host);
can we add at least DCHECK()s (or even CHECKs()) that the frame_host is known?

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:107:
NOTREACHED();
if you decided to go for a CHECK() above, this should also CHECK(false)

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:149:
return it == frame_host_state_map_.end() || it->second.error_occurred;
it == end() should be NOTREACHED()

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:166:
return it == frame_host_state_map_.end() || !it->second.is_navigating;
same here

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:183:
return it == frame_host_state_map_.end() || !it->second.is_parsing;
and here

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:202:
return it != frame_host_state_map_.end() && it->second.is_committed;
and here

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:219:
return it != frame_host_state_map_.end() && it->second.is_server_redirected;
...and here :)

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/web_navigation/frame_navigation_state_unittest.cc
(right):

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state_unittest.cc:16:
};
private: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc
(right):

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:67:
// TODO(dcheng): Is it actually OK to omit the parent process ID here?
can you file a bug for this (exposing the parent process ID)?

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.cc:144: }
nit. clang-for,at

[dcheng, 2014-07-14 20:12:56]

Note that I ended up just removing the WebContentsObserver::FrameDetached
override and moved the implementation completely into RenderFrameDetached().
Keeping both means that the new DCHECKs() I added in
FrameNavigationState::FrameDetached() will fire otherwise.

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc
(right):

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:82:
frame_host_state_map_.erase(frame_host);
On 2014/07/14 09:38:36, jochen wrote:
> can we add at least DCHECK()s (or even CHECKs()) that the frame_host is known?

Done.

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:82:
frame_host_state_map_.erase(frame_host);
On 2014/07/14 09:38:36, jochen wrote:
> can we add at least DCHECK()s (or even CHECKs()) that the frame_host is known?

Done.

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:107:
NOTREACHED();
On 2014/07/14 09:38:36, jochen wrote:
> if you decided to go for a CHECK() above, this should also CHECK(false)

I've opted for DCHECKs/NOTREACHED.

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:149:
return it == frame_host_state_map_.end() || it->second.error_occurred;
On 2014/07/14 09:38:36, jochen wrote:
> it == end() should be NOTREACHED()

Done.

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:166:
return it == frame_host_state_map_.end() || !it->second.is_navigating;
On 2014/07/14 09:38:36, jochen wrote:
> same here

Done.

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:183:
return it == frame_host_state_map_.end() || !it->second.is_parsing;
On 2014/07/14 09:38:36, jochen wrote:
> and here

Done.

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:202:
return it != frame_host_state_map_.end() && it->second.is_committed;
On 2014/07/14 09:38:36, jochen wrote:
> and here

Done.

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:219:
return it != frame_host_state_map_.end() && it->second.is_server_redirected;
On 2014/07/14 09:38:36, jochen wrote:
> ...and here :)

Done.

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/web_navigation/frame_navigation_state_unittest.cc
(right):

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/frame_navigation_state_unittest.cc:16:
};
On 2014/07/14 09:38:36, jochen wrote:
> private: DISALLOW_COPY_AND_ASSIGN

The base class (content::RenderViewHostTestHarness) already disallows this, but
done.

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc
(right):

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:67:
// TODO(dcheng): Is it actually OK to omit the parent process ID here?
On 2014/07/14 09:38:36, jochen wrote:
> can you file a bug for this (exposing the parent process ID)?

I actually wasn't sure if we needed this. My recollection is in cases where the
opener isn't disowned, we try to keep things in the same process... of course,
this will probably change with OOPIF, so bug filed =)

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/384993004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.cc:144: }
On 2014/07/14 09:38:36, jochen wrote:
> nit. clang-for,at

Done.

[Matt Perry, 2014-07-14 20:31:37]

lgtm

[dcheng, 2014-07-15 04:13:11]

Another update: it turns out the DCHECKs in FrameNavigationState::FrameDetached
fire a lot in various browser tests. At the end of the day, this is
fundamentally unavoidable if we start tracking frames in
DidStartProvisionalLoadForFrame and stop tracking them in FrameDetached. It is
possible for a frame to be created and detached without ever starting a
provisional load. There aren't a lot of options here:

1) Don't do anything in RenderFrameDeleted. This has the unfortunate side effect
of causing random use-after-frees. So probably this is a no go.
2) Remove the DCHECKs.
3) Keep the DCHECKs, rewrite FrameNavigationState so that we start tracking
frames in RenderFrameCreated (but perhaps somehow mark the entry as
'uninitialized' so the accessors can DCHECK if they are accessed too 'early')
and then have DidStartProvisionalLoadForFrame update it with the appropriate
URL/state, etc.

I personally prefer (2) because it's the simplest for me. However, if you prefer
(3), I would be happy to implement it in a followup patch as this one is already
pretty large as it is.

[jochen (gone - plz use gerrit), 2014-07-15 11:40:41]

removing the DCHECKs from FrameNavigationState::FrameDetached() only lgtm

[dcheng, 2014-07-15 18:01:29]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-07-15 18:06:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcheng@chromium.org/384993004/120001

[commit-bot: I haz the power, 2014-07-16 04:43:30]

Change committed as 283336