Issue 1661423002: Solidify Entry discarding logic (NavigationHandle keeps its ID)

Charlie Harrison

Description was changed from ========== [WIP] NavigationHandle holds NavigationEntry's unique id BUG= ========== to ========== ...

4 years, 10 months ago (2016-02-04 17:33:43 UTC) #1

Charlie Harrison

csharrison@chromium.org changed reviewers: + clamy@chromium.org, creis@chromium.org

4 years, 10 months ago (2016-02-04 21:48:42 UTC) #2

Charlie Harrison

PTAL. This is a WIP and I'm not 100% familiar with NavigationEntries, but it does ...

4 years, 10 months ago (2016-02-04 21:48:42 UTC) #3

Charlie Harrison

Description was changed from ========== [WIP] NavigationHandle holds NavigationEntry's unique id BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ...

4 years, 10 months ago (2016-02-04 21:49:09 UTC) #4

Charlie Harrison

Description was changed from ========== [WIP] NavigationHandle holds NavigationEntry's unique id BUG=513742 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ...

4 years, 10 months ago (2016-02-05 13:31:30 UTC) #5

Charlie Harrison

On 2016/02/04 at 21:48:42, csharrison wrote: > PTAL. This is a WIP and I'm not ...

4 years, 10 months ago (2016-02-05 15:00:59 UTC) #6

Charlie Reis

Thanks for tackling this! I'm going to depend on Camille (and/or Nasko) to verify that ...

4 years, 10 months ago (2016-02-05 19:36:12 UTC) #7

Charlie Harrison

Thanks for the review! I want this change to be good with PlzNavigate, so I ...

4 years, 10 months ago (2016-02-05 23:10:24 UTC) #8

Thanks for the review!

I want this change to be good with PlzNavigate, so I still am a little undecided
about how to assign nav ids to handles under PlzNavigate.

I know the RequestNavigationParams have the nav_id set to 0 if the navigation is
renderer initiated. It would be nice if the entry associated with *any*
navigation could be accessed via the NavigationRequest (not just browser
initiated ones).

That would mean we wouldn't have to input the Entry into CreateNavigationHandle,
the NavigationRequest would do it all for us (like everything else associated
with a handle).

clamy@, would appreciate your thoughts here.

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:387:
TEST_F(NavigationControllerTest, DontDiscardWrongPendingEntry) {
On 2016/02/05 at 19:36:11, Charlie Reis wrote:
> nit: Let's add a comment with the bug number and a brief description,
including your repro steps:
> 1. Pending entry for A is created
> 2. DidStartProvisionalLoad message for A
> 3. Pending entry for B is created
> 4. DidFailProvisionalLoad message for A
> 5. DidStartProvisionalLoad message for B

Done.

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:413:
EXPECT_EQ(controller.GetVisibleEntry()->GetURL(), url_2);
On 2016/02/05 at 19:36:11, Charlie Reis wrote:
> nit: Copy this EXPECT_EQ after the LoadURL(url_2) and the
SimulateNavigationError.
> 
> Maybe also have it for url_1 after the SimulateNavigationStart(url_1)?

Done.

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:47: NavigationEntry*
navigation_entry)
On 2016/02/05 at 19:36:11, Charlie Reis wrote:
> If we're only using the NavEntry's ID, maybe we should just pass that in and
not have this class depend on NavigationEntry itself?

My argument would be that there now *is* a real dependency between this class
and NavigationEntry, and this just makes that relationship explicit and
statically verifiable. Feel free to override though :)

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:61:
entry_id_(navigation_entry ? navigation_entry->GetUniqueID() : -1) {
On 2016/02/05 at 19:36:11, Charlie Reis wrote:
> nit: We use 0 for none (as in RenderFrameHostImpl::nav_entry_id_).

Done.

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:69: // associated with this
navigation.
On 2016/02/05 at 19:36:11, Charlie Reis wrote:
> Need to document what cases it's ok for the NavEntry to be null.  Am I right
that it is only null for synchronous navigations, when there is no pending
NavEntry?

Yeah, that what I gathered. Perhaps the nav_entry_id should be renamed to
pending_nav_entry_id? The navigation handle goes away after commit, after all.

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:116: int get_entry_id() {
return entry_id_; }
On 2016/02/05 at 19:36:11, Charlie Reis wrote:
> nit: Maybe nav_entry_id(), as in RenderFrameHostImpl?
> 
> This would also benefit from a comment about when it's valid, that it's 0 for
invalid, and whether it can change over time.

I don't *think* it can change over time, so I'll mark it const (and make the
function const too, why not) + added a comment.

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl_unittest.cc (right):

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_unittest.cc:58:
base::TimeTicks::Now(), nullptr);
On 2016/02/05 at 19:36:12, Charlie Reis wrote:
> Should we have some tests for the non-null case?

Not sure. These tests (afaict) don't really do much integration, so it isn't
clear what to actually set this as. clamy@, thoughts?

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:131: void
CreateNavigationHandle(NavigationEntry* navigation_entry);
On 2016/02/05 at 19:36:12, Charlie Reis wrote:
> As before, we should have a comment about this parameter (e.g., when it's
expected to be valid).

see global comment.

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:219: NavigationEntry* pending =
controller_->GetPendingEntry();
On 2016/02/05 at 19:36:12, Charlie Reis wrote:
> nit: pending_entry

Done.

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:220: bool pending_matches_fail =
On 2016/02/05 at 19:36:12, Charlie Reis wrote:
> nit: pending_entry_matches_fail_msg

Done.

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:225: pending_matches_fail) {
On 2016/02/05 at 19:36:12, Charlie Reis wrote:
> This is really hard to follow.  (It was already really hard to follow, and now
I think it's past the threshold that I can reason about it.)  :)
> 
> I think we're basically saying: do what we did before, but make sure we only
discard the pending entry here if the DidFail message matches the pending entry.
> 
> Maybe this would be more legible like:
> 
>   // Big comment about why we only care if the pending entry matches the IPC
>   NavHandle handle...
>   if (pending_entry_matches_fail_msg) {
>     // We usually...
>     bool should_preserve...
>     if (pending_entry != ...) {
>       Discard + Notify
>     }
>   }

SGTM. My projections show that by next year, this method will read as a tidy
novella!

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:795:
navigation_request->CreateNavigationHandle(controller_->GetPendingEntry());
On 2016/02/05 at 19:36:12, Charlie Reis wrote:
> nit: Comment about why this must be after DidStartMainFrameNavigation, and no
blank line after.

Done.

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:949:
navigation_request->CreateNavigationHandle(controller_->GetVisibleEntry());
On 2016/02/05 at 19:36:12, Charlie Reis wrote:
> Shouldn't this be GetPendingEntry()?  GetVisibleEntry() can return the last
committed entry in situations that we don't think it's safe to display the
pending one (e.g., renderer-initiated navigations).

Yes. Good catch. In fact, here we can use the NavigationEntryImpl that was
passed into the method.

Charlie Reis

Seems pretty good to me. I'll defer to Camille on the PlzNavigate behavior, and I've ...

4 years, 10 months ago (2016-02-06 00:54:58 UTC) #9

Seems pretty good to me.  I'll defer to Camille on the PlzNavigate behavior, and
I've got one sanity check below to be sure we don't introduce a URL spoof
vulnerability.

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:47: NavigationEntry*
navigation_entry)
On 2016/02/05 23:10:23, csharrison wrote:
> On 2016/02/05 at 19:36:11, Charlie Reis wrote:
> > If we're only using the NavEntry's ID, maybe we should just pass that in and
> not have this class depend on NavigationEntry itself?
> 
> My argument would be that there now *is* a real dependency between this class
> and NavigationEntry, and this just makes that relationship explicit and
> statically verifiable. Feel free to override though :)

I see your point, but I feel like this tempts us to use more and more data from
the NavigationEntry over time, and makes it unclear how much NavigationHandle
needs to know (e.g., should it care if a new value gets added to NavEntry?).  It
also looks like |url| becomes redundant with the NavEntry's URL, but in some
cases we don't have a NavEntry.

I'd prefer to just state what's needed in the contract, which would be the ID.

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:69: // associated with this
navigation.
On 2016/02/05 23:10:23, csharrison wrote:
> On 2016/02/05 at 19:36:11, Charlie Reis wrote:
> > Need to document what cases it's ok for the NavEntry to be null.  Am I right
> that it is only null for synchronous navigations, when there is no pending
> NavEntry?
> 
> Yeah, that what I gathered. Perhaps the nav_entry_id should be renamed to
> pending_nav_entry_id? The navigation handle goes away after commit, after all.

Yes, I like pending_nav_entry_id.

https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:398: // the
omnibox, which is the entry before A was created.
Perfect.  Tiny nit: We usually put these comments above the TEST_F line.

https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:116: // Get the
corresponding id from the NavigationEntry associated with this
nit: s/corresponding/unique/

https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:118: // will not have a
valid id. It will be 0.
nit: will not have a NavigationEntry associated with it, and this will return 0.

https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:203: NavigationHandleImpl* handle =
render_frame_host->navigation_handle();
nit: Please put these declarations below the big comment, since the comment
helps explain them.

https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:208: // Another complicated case
here is the fact that racy conditions can cause a
nit: Drop "Another complicated case here is the fact that"

(This comes before the rest of the explanation, so using "Another" doesn't make
sense to the reader.  Clearer to just start with "Racy conditions can cause...")

https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:215: if (pending_matches_fail_msg)
{
Sanity check: I'm a bit nervous about unintentionally allowing URL spoofs if an
attacker can somehow clear the handle or make it not match the pending entry. 
In that case, the pending entry would be left around when we otherwise would
have discarded it.

(I'm also trying to remember the attack that's possible if the pending entry
doesn't match the visible entry and we leave the pending entry around.  I might
have to dig through source history to remind myself.)

I'm hoping we can verify that there isn't an attack possible here.

Charlie Harrison

On 2016/02/06 00:54:58, Charlie Reis wrote: > Seems pretty good to me. I'll defer to ...

4 years, 10 months ago (2016-02-07 00:05:28 UTC) #10

On 2016/02/06 00:54:58, Charlie Reis wrote:
> Seems pretty good to me.  I'll defer to Camille on the PlzNavigate behavior,
and
> I've got one sanity check below to be sure we don't introduce a URL spoof
> vulnerability.
> 
>
https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
> File content/browser/frame_host/navigation_handle_impl.cc (right):
> 
>
https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
> content/browser/frame_host/navigation_handle_impl.cc:47: NavigationEntry*
> navigation_entry)
> On 2016/02/05 23:10:23, csharrison wrote:
> > On 2016/02/05 at 19:36:11, Charlie Reis wrote:
> > > If we're only using the NavEntry's ID, maybe we should just pass that in
and
> > not have this class depend on NavigationEntry itself?
> > 
> > My argument would be that there now *is* a real dependency between this
class
> > and NavigationEntry, and this just makes that relationship explicit and
> > statically verifiable. Feel free to override though :)
> 
> I see your point, but I feel like this tempts us to use more and more data
from
> the NavigationEntry over time, and makes it unclear how much NavigationHandle
> needs to know (e.g., should it care if a new value gets added to NavEntry?). 
It
> also looks like |url| becomes redundant with the NavEntry's URL, but in some
> cases we don't have a NavEntry.
> 
> I'd prefer to just state what's needed in the contract, which would be the ID.
> 
>
https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
> File content/browser/frame_host/navigation_handle_impl.h (right):
> 
>
https://codereview.chromium.org/1661423002/diff/40001/content/browser/frame_h...
> content/browser/frame_host/navigation_handle_impl.h:69: // associated with
this
> navigation.
> On 2016/02/05 23:10:23, csharrison wrote:
> > On 2016/02/05 at 19:36:11, Charlie Reis wrote:
> > > Need to document what cases it's ok for the NavEntry to be null.  Am I
right
> > that it is only null for synchronous navigations, when there is no pending
> > NavEntry?
> > 
> > Yeah, that what I gathered. Perhaps the nav_entry_id should be renamed to
> > pending_nav_entry_id? The navigation handle goes away after commit, after
all.
> 
> Yes, I like pending_nav_entry_id.
> 
>
https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
> File content/browser/frame_host/navigation_controller_impl_unittest.cc
(right):
> 
>
https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
> content/browser/frame_host/navigation_controller_impl_unittest.cc:398: // the
> omnibox, which is the entry before A was created.
> Perfect.  Tiny nit: We usually put these comments above the TEST_F line.
> 
>
https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
> File content/browser/frame_host/navigation_handle_impl.h (right):
> 
>
https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
> content/browser/frame_host/navigation_handle_impl.h:116: // Get the
> corresponding id from the NavigationEntry associated with this
> nit: s/corresponding/unique/
> 
>
https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
> content/browser/frame_host/navigation_handle_impl.h:118: // will not have a
> valid id. It will be 0.
> nit: will not have a NavigationEntry associated with it, and this will return
0.
> 
>
https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
> File content/browser/frame_host/navigator_impl.cc (right):
> 
>
https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
> content/browser/frame_host/navigator_impl.cc:203: NavigationHandleImpl* handle
=
> render_frame_host->navigation_handle();
> nit: Please put these declarations below the big comment, since the comment
> helps explain them.
> 
>
https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
> content/browser/frame_host/navigator_impl.cc:208: // Another complicated case
> here is the fact that racy conditions can cause a
> nit: Drop "Another complicated case here is the fact that"
> 
> (This comes before the rest of the explanation, so using "Another" doesn't
make
> sense to the reader.  Clearer to just start with "Racy conditions can
cause...")
> 
>
https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
> content/browser/frame_host/navigator_impl.cc:215: if
(pending_matches_fail_msg)
> {
> Sanity check: I'm a bit nervous about unintentionally allowing URL spoofs if
an
> attacker can somehow clear the handle or make it not match the pending entry. 
> In that case, the pending entry would be left around when we otherwise would
> have discarded it.
> 
> (I'm also trying to remember the attack that's possible if the pending entry
> doesn't match the visible entry and we leave the pending entry around.  I
might
> have to dig through source history to remind myself.)
> 
> I'm hoping we can verify that there isn't an attack possible here.

I think this was your previous spoof bug:
https://code.google.com/p/chromium/issues/detail?id=280512

From the bug, it seems the only thing we should worry about are in-page navs
(rest are handled by DidStartProvisionalLoad). The problem happens when an
in-page  navigation B aborts a previous navigation A, and re-uses A's virtual
url.

In this case, the pending entry should be A, which gets discarded so it can't be
spoofed.

In this patch, we'd like the logic to be the same, and have A be discarded
properly. This means that the current navigation handle should match A's entry
at this point. I believe this must happen, because the nav handle is updated for
in-page navigations on OnDidCommitProvisionalLoad. That happens after the
DidFailProvisionalLoad, as in blink the provisional loads are failed before the
in-page navigation is sent out:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Are there any other possible points where A's navigation handle could be
destroyed before its Entry is discarded?

Two places I can think of are
RenderFrameHostImpl::OnDidStopLoading
RenderFrameHostImpl::OnDidDropNavigation

I haven't figured out if those safely remove the pending entry.

clamy

Thanks! The PlzNavigate part seems ok to me. I'll do another review once Charlie's comments ...

4 years, 10 months ago (2016-02-08 13:46:23 UTC) #11

Charlie Harrison

Thanks clamy@! creis@, Let me know your thoughts on the spoofing side of this. There ...

4 years, 10 months ago (2016-02-08 15:06:42 UTC) #12

Charlie Reis

On 2016/02/07 00:05:28, csharrison wrote: https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_host/navigator_impl.cc#newcode215 > > content/browser/frame_host/navigator_impl.cc:215: if > (pending_matches_fail_msg) > > { ...

4 years, 10 months ago (2016-02-08 19:44:06 UTC) #13

On 2016/02/07 00:05:28, csharrison wrote:
https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
> > content/browser/frame_host/navigator_impl.cc:215: if
> (pending_matches_fail_msg)
> > {
> > Sanity check: I'm a bit nervous about unintentionally allowing URL spoofs if
> an
> > attacker can somehow clear the handle or make it not match the pending
entry. 
> > In that case, the pending entry would be left around when we otherwise would
> > have discarded it.
> > 
> > (I'm also trying to remember the attack that's possible if the pending entry
> > doesn't match the visible entry and we leave the pending entry around.  I
> might
> > have to dig through source history to remind myself.)
> > 
> > I'm hoping we can verify that there isn't an attack possible here.
> 
> I think this was your previous spoof bug:
> https://code.google.com/p/chromium/issues/detail?id=280512
> 
> From the bug, it seems the only thing we should worry about are in-page navs
> (rest are handled by DidStartProvisionalLoad). The problem happens when an
> in-page  navigation B aborts a previous navigation A, and re-uses A's virtual
> url.
> 
> In this case, the pending entry should be A, which gets discarded so it can't
be
> spoofed.

Thanks!  That jogs my memory, and I think we'll be able to do even better there
with your change.

The tricky part for that spoof was that
NavigationControllerImpl::RendererDidNavigate does a clone of the pending entry
to create the new last committed entry.  That copies over the virtual URL from
the (possibly incorrect) pending entry when the new URL commits.  Note that that
code has a weak sanity check that the pending entry "matches" (based only on
SiteInstance):
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

With your change, we can (probably) actually verify whether the pending entry
matches the committed navigation or not, when deciding whether to clone.  Do we
have access to the NavigationHandle at the time of
NavigationControllerImpl::RendererDidNavigate?

Not sure if we want to tackle that cleanup here as well, or in a separate CL.  I
think it'll be worthwhile, though.

> 
> In this patch, we'd like the logic to be the same, and have A be discarded
> properly. This means that the current navigation handle should match A's entry
> at this point. I believe this must happen, because the nav handle is updated
for
> in-page navigations on OnDidCommitProvisionalLoad. That happens after the
> DidFailProvisionalLoad, as in blink the provisional loads are failed before
the
> in-page navigation is sent out:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 

Great, I agree about that case.

> Are there any other possible points where A's navigation handle could be
> destroyed before its Entry is discarded?
> 
> Two places I can think of are
> RenderFrameHostImpl::OnDidStopLoading
> RenderFrameHostImpl::OnDidDropNavigation
> 
> I haven't figured out if those safely remove the pending entry.

Nice find!  Those don't appear to remove the pending entry.  I bet the
DidDropNavigation one could be turned into a spoof.  (We're hoping to remove
that IPC entirely, but I think it's still used for an error case.  If we can
figure out which type of error, that might give us repro steps.)

Guess this is another reason to add the NavHandle check to
RendererDidNavigateToNewPage!

https://codereview.chromium.org/1661423002/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1661423002/diff/120001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:204: // Racy conditions can cause a
 fail message to arrive after its corresponding
nit: Remove extra space before "fail."
Same in each line below.

Charlie Harrison

On 2016/02/08 19:44:06, Charlie Reis wrote: > On 2016/02/07 00:05:28, csharrison wrote: > https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_host/navigator_impl.cc#newcode215 > ...

4 years, 10 months ago (2016-02-08 20:07:55 UTC) #14

On 2016/02/08 19:44:06, Charlie Reis wrote:
> On 2016/02/07 00:05:28, csharrison wrote:
>
https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
> > > content/browser/frame_host/navigator_impl.cc:215: if
> > (pending_matches_fail_msg)
> > > {
> > > Sanity check: I'm a bit nervous about unintentionally allowing URL spoofs
if
> > an
> > > attacker can somehow clear the handle or make it not match the pending
> entry. 
> > > In that case, the pending entry would be left around when we otherwise
would
> > > have discarded it.
> > > 
> > > (I'm also trying to remember the attack that's possible if the pending
entry
> > > doesn't match the visible entry and we leave the pending entry around.  I
> > might
> > > have to dig through source history to remind myself.)
> > > 
> > > I'm hoping we can verify that there isn't an attack possible here.
> > 
> > I think this was your previous spoof bug:
> > https://code.google.com/p/chromium/issues/detail?id=280512
> > 
> > From the bug, it seems the only thing we should worry about are in-page navs
> > (rest are handled by DidStartProvisionalLoad). The problem happens when an
> > in-page  navigation B aborts a previous navigation A, and re-uses A's
virtual
> > url.
> > 
> > In this case, the pending entry should be A, which gets discarded so it
can't
> be
> > spoofed.
> 
> Thanks!  That jogs my memory, and I think we'll be able to do even better
there
> with your change.
> 
> The tricky part for that spoof was that
> NavigationControllerImpl::RendererDidNavigate does a clone of the pending
entry
> to create the new last committed entry.  That copies over the virtual URL from
> the (possibly incorrect) pending entry when the new URL commits.  Note that
that
> code has a weak sanity check that the pending entry "matches" (based only on
> SiteInstance):
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> 
> With your change, we can (probably) actually verify whether the pending entry
> matches the committed navigation or not, when deciding whether to clone.  Do
we
> have access to the NavigationHandle at the time of
> NavigationControllerImpl::RendererDidNavigate?
> 
> Not sure if we want to tackle that cleanup here as well, or in a separate CL. 
I
> think it'll be worthwhile, though.
> 
> 
> > 
> > In this patch, we'd like the logic to be the same, and have A be discarded
> > properly. This means that the current navigation handle should match A's
entry
> > at this point. I believe this must happen, because the nav handle is updated
> for
> > in-page navigations on OnDidCommitProvisionalLoad. That happens after the
> > DidFailProvisionalLoad, as in blink the provisional loads are failed before
> the
> > in-page navigation is sent out:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> 
> Great, I agree about that case.
> 
> > Are there any other possible points where A's navigation handle could be
> > destroyed before its Entry is discarded?
> > 
> > Two places I can think of are
> > RenderFrameHostImpl::OnDidStopLoading
> > RenderFrameHostImpl::OnDidDropNavigation
> > 
> > I haven't figured out if those safely remove the pending entry.
> 
> Nice find!  Those don't appear to remove the pending entry.  I bet the
> DidDropNavigation one could be turned into a spoof.  (We're hoping to remove
> that IPC entirely, but I think it's still used for an error case.  If we can
> figure out which type of error, that might give us repro steps.)
> 
> Guess this is another reason to add the NavHandle check to
> RendererDidNavigateToNewPage!
> 
>
https://codereview.chromium.org/1661423002/diff/120001/content/browser/frame_...
> File content/browser/frame_host/navigator_impl.cc (right):
> 
>
https://codereview.chromium.org/1661423002/diff/120001/content/browser/frame_...
> content/browser/frame_host/navigator_impl.cc:204: // Racy conditions can cause
a
>  fail message to arrive after its corresponding
> nit: Remove extra space before "fail."
> Same in each line below.

RendererDidNavigate should have the nav handle. It's called from
NavigatorImpl::DidNavigate which is responsible for sending the
DidFinishNavigation message to observers (by deleting the handle).

I'll see what I can do wrt improving the check there.

Charlie Harrison

On 2016/02/08 20:07:55, csharrison wrote: > On 2016/02/08 19:44:06, Charlie Reis wrote: > > On ...

4 years, 10 months ago (2016-02-08 23:02:52 UTC) #15

On 2016/02/08 20:07:55, csharrison wrote:
> On 2016/02/08 19:44:06, Charlie Reis wrote:
> > On 2016/02/07 00:05:28, csharrison wrote:
> >
>
https://codereview.chromium.org/1661423002/diff/60001/content/browser/frame_h...
> > > > content/browser/frame_host/navigator_impl.cc:215: if
> > > (pending_matches_fail_msg)
> > > > {
> > > > Sanity check: I'm a bit nervous about unintentionally allowing URL
spoofs
> if
> > > an
> > > > attacker can somehow clear the handle or make it not match the pending
> > entry. 
> > > > In that case, the pending entry would be left around when we otherwise
> would
> > > > have discarded it.
> > > > 
> > > > (I'm also trying to remember the attack that's possible if the pending
> entry
> > > > doesn't match the visible entry and we leave the pending entry around. 
I
> > > might
> > > > have to dig through source history to remind myself.)
> > > > 
> > > > I'm hoping we can verify that there isn't an attack possible here.
> > > 
> > > I think this was your previous spoof bug:
> > > https://code.google.com/p/chromium/issues/detail?id=280512
> > > 
> > > From the bug, it seems the only thing we should worry about are in-page
navs
> > > (rest are handled by DidStartProvisionalLoad). The problem happens when an
> > > in-page  navigation B aborts a previous navigation A, and re-uses A's
> virtual
> > > url.
> > > 
> > > In this case, the pending entry should be A, which gets discarded so it
> can't
> > be
> > > spoofed.
> > 
> > Thanks!  That jogs my memory, and I think we'll be able to do even better
> there
> > with your change.
> > 
> > The tricky part for that spoof was that
> > NavigationControllerImpl::RendererDidNavigate does a clone of the pending
> entry
> > to create the new last committed entry.  That copies over the virtual URL
from
> > the (possibly incorrect) pending entry when the new URL commits.  Note that
> that
> > code has a weak sanity check that the pending entry "matches" (based only on
> > SiteInstance):
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> > 
> > With your change, we can (probably) actually verify whether the pending
entry
> > matches the committed navigation or not, when deciding whether to clone.  Do
> we
> > have access to the NavigationHandle at the time of
> > NavigationControllerImpl::RendererDidNavigate?
> > 
> > Not sure if we want to tackle that cleanup here as well, or in a separate
CL. 
> I
> > think it'll be worthwhile, though.
> > 
> > 
> > > 
> > > In this patch, we'd like the logic to be the same, and have A be discarded
> > > properly. This means that the current navigation handle should match A's
> entry
> > > at this point. I believe this must happen, because the nav handle is
updated
> > for
> > > in-page navigations on OnDidCommitProvisionalLoad. That happens after the
> > > DidFailProvisionalLoad, as in blink the provisional loads are failed
before
> > the
> > > in-page navigation is sent out:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > 
> > 
> > Great, I agree about that case.
> > 
> > > Are there any other possible points where A's navigation handle could be
> > > destroyed before its Entry is discarded?
> > > 
> > > Two places I can think of are
> > > RenderFrameHostImpl::OnDidStopLoading
> > > RenderFrameHostImpl::OnDidDropNavigation
> > > 
> > > I haven't figured out if those safely remove the pending entry.
> > 
> > Nice find!  Those don't appear to remove the pending entry.  I bet the
> > DidDropNavigation one could be turned into a spoof.  (We're hoping to remove
> > that IPC entirely, but I think it's still used for an error case.  If we can
> > figure out which type of error, that might give us repro steps.)
> > 
> > Guess this is another reason to add the NavHandle check to
> > RendererDidNavigateToNewPage!
> > 
> >
>
https://codereview.chromium.org/1661423002/diff/120001/content/browser/frame_...
> > File content/browser/frame_host/navigator_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1661423002/diff/120001/content/browser/frame_...
> > content/browser/frame_host/navigator_impl.cc:204: // Racy conditions can
cause
> a
> >  fail message to arrive after its corresponding
> > nit: Remove extra space before "fail."
> > Same in each line below.
> 
> RendererDidNavigate should have the nav handle. It's called from
> NavigatorImpl::DidNavigate which is responsible for sending the
> DidFinishNavigation message to observers (by deleting the handle).
> 
> I'll see what I can do wrt improving the check there.

Unfortunately if we compare handles at RendererDidNavigate, it causes two tests
to fail that require updating the virtual url there.

LoadDataWithBaseURL
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

NavigateFromLoadDataWithBaseURL
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

These are data loads, so what I think happens is that they come back from the
renderer as synchronous, same-page navs. So because same page navs get a pending
entry id of 0, they can't update their virtual urls.

Charlie Reis

On 2016/02/08 23:02:52, csharrison wrote: > Unfortunately if we compare handles at RendererDidNavigate, it causes ...

4 years, 10 months ago (2016-02-08 23:23:43 UTC) #16

Charlie Harrison

On 2016/02/08 23:23:43, Charlie Reis wrote: > On 2016/02/08 23:02:52, csharrison wrote: > > Unfortunately ...

4 years, 10 months ago (2016-02-09 13:33:28 UTC) #17

On 2016/02/08 23:23:43, Charlie Reis wrote:
> On 2016/02/08 23:02:52, csharrison wrote:
> > Unfortunately if we compare handles at RendererDidNavigate, it causes two
> tests
> > to fail that require updating the virtual url there.
> > 
> > LoadDataWithBaseURL
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> > 
> > NavigateFromLoadDataWithBaseURL
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> > 
> > These are data loads, so what I think happens is that they come back from
the
> > renderer as synchronous, same-page navs. So because same page navs get a
> pending
> > entry id of 0, they can't update their virtual urls.
> 
> Are you sure that's the reason?  Data URL shouldn't be synchronous, and it
looks
> like we're getting to DidStartProvisionalLoad for them.
> 
> The whole LoadDataWithBaseURL feature is a hack for Android Webview, so it
might
> behave differently than most navigations.  I wonder if we can find a way to
bend
> it to work here.
> 
> In this case, maybe we're creating a second pending entry and copying things
> over?  It looks like NavigatorImpl::DidStartMainFrameNavigation is guilty of
> that for other cases, but I'm not sure about this one.

I was mistaken. The real reason is that the navigation handle associated with
the data load originally tracks the base_url.

When the load commits, we compare validated_params.url with the navigation
handle's url. This returns false (I guess for all data urls with base urls?), so
we replace the navigation handle, assuming the loads don't match.

This is why I thought that there was a synchronous renderer-initiated load,
because this case follows that same path for them.

There are two obvious solutions to this:
1. Don't change semantics with NavigationHandle (even though it seems broken).
Just propagate the entry_id to the next NavigationHandle when we hit this odd
case. This hack should probably be temporary until NavHandles are fixed.
2. Fix nav handles right away. I'm not sure the right way to do this. Somehow
they are getting assigned with the base url, when they should be assigned the
data url. This is probably an issue with the requests in blink holding the wrong
(?) thing in their |url| member.

I can probably look into (2) today, but (1) is an easy fix and all tests pass
locally with the added guard for virtual urls.

Charlie Reis

On 2016/02/09 13:33:28, csharrison wrote: > On 2016/02/08 23:23:43, Charlie Reis wrote: > > On ...

4 years, 10 months ago (2016-02-10 00:25:42 UTC) #18

On 2016/02/09 13:33:28, csharrison wrote:
> On 2016/02/08 23:23:43, Charlie Reis wrote:
> > On 2016/02/08 23:02:52, csharrison wrote:
> > > Unfortunately if we compare handles at RendererDidNavigate, it causes two
> > tests
> > > to fail that require updating the virtual url there.
> > > 
> > > LoadDataWithBaseURL
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> > > 
> > > NavigateFromLoadDataWithBaseURL
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> > > 
> > > These are data loads, so what I think happens is that they come back from
> the
> > > renderer as synchronous, same-page navs. So because same page navs get a
> > pending
> > > entry id of 0, they can't update their virtual urls.
> > 
> > Are you sure that's the reason?  Data URL shouldn't be synchronous, and it
> looks
> > like we're getting to DidStartProvisionalLoad for them.
> > 
> > The whole LoadDataWithBaseURL feature is a hack for Android Webview, so it
> might
> > behave differently than most navigations.  I wonder if we can find a way to
> bend
> > it to work here.
> > 
> > In this case, maybe we're creating a second pending entry and copying things
> > over?  It looks like NavigatorImpl::DidStartMainFrameNavigation is guilty of
> > that for other cases, but I'm not sure about this one.
> 
> I was mistaken. The real reason is that the navigation handle associated with
> the data load originally tracks the base_url.
> 
> When the load commits, we compare validated_params.url with the navigation
> handle's url. This returns false (I guess for all data urls with base urls?),
so
> we replace the navigation handle, assuming the loads don't match.

Oh, wow.  That's unfortunate.  We don't have a similar problem for other virtual
URLs, do we?  (Try chrome://settings, which is actually a virtual URL that we
display for chrome://chrome/settings/ under the hood.)

> This is why I thought that there was a synchronous renderer-initiated load,
> because this case follows that same path for them.
> 
> There are two obvious solutions to this:
> 1. Don't change semantics with NavigationHandle (even though it seems broken).
> Just propagate the entry_id to the next NavigationHandle when we hit this odd
> case. This hack should probably be temporary until NavHandles are fixed.
> 2. Fix nav handles right away. I'm not sure the right way to do this. Somehow
> they are getting assigned with the base url, when they should be assigned the
> data url. This is probably an issue with the requests in blink holding the
wrong
> (?) thing in their |url| member.
> 
> I can probably look into (2) today, but (1) is an easy fix and all tests pass
> locally with the added guard for virtual urls.

If this affects virtual URLs like chrome://settings, then we should tackle (2)
ASAP (in a separate CL).  If not, we can probably tackle it later and do a
version of (1) (hopefully without resetting the NavHandle at all).

https://codereview.chromium.org/1661423002/diff/140001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1661423002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1088: // that was just
loaded. Verify this by checking if the entry corresponds
Glad we're adding this!  Can you add a mention of it to the CL description? 
(Maybe we can remove the 280512 logic from DidFailProvisionalLoad after all this
lands.)

https://codereview.chromium.org/1661423002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1093: // history
navigation that was interrupted by an unreleated,
nit: unrelated

https://codereview.chromium.org/1661423002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1100:
pending_entry_->site_instance() == rfh->GetSiteInstance())) {
Side note: I'm not sure if the SiteInstances can differ if the NavHandle and
pending entry have the same unique ID, can they?

That said, I'm happy leaving this as part of the check for now (to be safe), and
maybe later turning it into an assertion.

https://codereview.chromium.org/1661423002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1661423002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1012: if
(navigation_handle_->GetURL() == validated_params.base_url)
Why even reset the NavHandle in this case?  Creating a new one seems wrong to
me.

Charlie Harrison

Description was changed from ========== This patch gives the NavigationHandle a NavigationEntry on instantiation, so ...

4 years, 10 months ago (2016-02-10 01:07:02 UTC) #19

Charlie Harrison

Still unsure about the best solution for data urls w/ base urls. I think we ...

4 years, 10 months ago (2016-02-11 20:23:05 UTC) #20

Still unsure about the best solution for data urls w/ base urls. I think we have
to someone send up a different / additional url in DidStartProvisionalLoad...

https://codereview.chromium.org/1661423002/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1661423002/diff/120001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:204: // Racy conditions can cause a
 fail message to arrive after its corresponding
On 2016/02/08 19:44:06, Charlie Reis wrote:
> nit: Remove extra space before "fail."
> Same in each line below.

Ugh. Relied too much on clang-format to clean this up.

https://codereview.chromium.org/1661423002/diff/140001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1661423002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1088: // that was just
loaded. Verify this by checking if the entry corresponds
On 2016/02/10 00:25:42, Charlie Reis wrote:
> Glad we're adding this!  Can you add a mention of it to the CL description? 
> (Maybe we can remove the 280512 logic from DidFailProvisionalLoad after all
this
> lands.)

Done.

https://codereview.chromium.org/1661423002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1093: // history
navigation that was interrupted by an unreleated,
On 2016/02/10 00:25:42, Charlie Reis wrote:
> nit: unrelated

Done.

https://codereview.chromium.org/1661423002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1100:
pending_entry_->site_instance() == rfh->GetSiteInstance())) {
On 2016/02/10 00:25:42, Charlie Reis wrote:
> Side note: I'm not sure if the SiteInstances can differ if the NavHandle and
> pending entry have the same unique ID, can they?
> 
> That said, I'm happy leaving this as part of the check for now (to be safe),
and
> maybe later turning it into an assertion.

I wanted to leave them in to be sure. I'm happy to leave a TODO :)

https://codereview.chromium.org/1661423002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1661423002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1012: if
(navigation_handle_->GetURL() == validated_params.base_url)
On 2016/02/10 00:25:42, Charlie Reis wrote:
> Why even reset the NavHandle in this case?  Creating a new one seems wrong to
> me.

It is wrong, but I think we have to do a more fundamental fix here, rather than
just not deleting these handles. The handle's url is expected to match the
params.url. This gets CHECKed in NavigationHandleImpl::DidCommitNavigation.

I think the handle should track the normal url, not the base url. I think this
happens here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I tried sending up a different url in DidStartProvisionalLoad (i.e. the original
url), but many tests failed. Not sure of the best solution here.

Charlie Harrison

I figured out how to handle base urls with a little bit of a hack. ...

4 years, 10 months ago (2016-02-16 20:54:46 UTC) #21

Charlie Reis

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

4 years, 10 months ago (2016-02-16 21:55:06 UTC) #22

Charlie Reis

[+boliu] Bo, can you take a look at the change to RenderFrameImpl::didStartProvisionalLoad? We would pass ...

4 years, 10 months ago (2016-02-16 21:55:07 UTC) #23

Charlie Harrison

Looks like we're getting consistent failures from the chromeos bot. The errors are strange too. ...

4 years, 10 months ago (2016-02-16 22:55:23 UTC) #24

Looks like we're getting consistent failures from the chromeos bot. The errors
are strange too. Only one out of three EXPECT statements are failing... I
believe this regressed when we mucked with the virtual urls, but I can't repro
locally.

https://codereview.chromium.org/1661423002/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/1661423002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:395: // At
step (4), the pending entry for B is discarded, when A is the one that
On 2016/02/16 21:55:07, Charlie Reis (catching up) wrote:
> s/is/used to be/

Done.

https://codereview.chromium.org/1661423002/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/1661423002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_request.h:131: // NavigationRequest for
the FrameTreeNode has been destroyed.
On 2016/02/16 21:55:07, Charlie Reis (catching up) wrote:
> nit: Let's add a similar comment about the ID (and when it's 0) as on
> NavigationHandleImpl::pending_nav_entry_id() itself.

Done.

https://codereview.chromium.org/1661423002/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1661423002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:236: if (pending_entry !=
controller_->GetVisibleEntry() ||
On 2016/02/16 21:55:07, Charlie Reis (catching up) wrote:
> Sanity check: If I remember correctly this line isn't critical anymore because
> of the check we've added to RendererDidNavigateToNewPage's clone logic, but
> we're keeping this around for now to be safe.  (Maybe add a TODO for me to
clean
> it up later?)

Done.

https://codereview.chromium.org/1661423002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1661423002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1009:
(navigation_handle_->GetURL() != validated_params.url)) {
On 2016/02/16 21:55:07, Charlie Reis (catching up) wrote:
> This change doesn't seem necessary, but I don't feel strongly either way. 
(The
> main cost is for people searching through git blame.)

Removed.

https://codereview.chromium.org/1661423002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1024: 0);
On 2016/02/16 21:55:07, Charlie Reis (catching up) wrote:
> nit: This fits on the previous line.  Call sites don't need to have each
> parameter on their own line.  (I'll defer if git cl format did this, but I'd
be
> surprised since the first line has two parameters as well.)

Done.

https://codereview.chromium.org/1661423002/diff/180001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1661423002/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3084: GURL url(ds->request().url());
On 2016/02/16 21:55:07, Charlie Reis (catching up) wrote:
> On 2016/02/16 20:54:46, csharrison wrote:
> > This snippet solves the base url / data url problem with us creating 2
> > NavigationHandles. If all tests pass, I assume no one is relying on us
sending
> > the other url.
> 
> This is largely a question for boliu@, since it might impact apps that use
> Android Webview.  I'm not sure if they have expectations about which URL shows
> up in DidStartProvisionalLoad.
> 
> (Also, please add a comment about the LoadDataWithBaseURL case, since it's not
> obvious why this is needed from looking at the code.)

Done.

boliu

boliu@chromium.org changed reviewers: + mnaganov@chromium.org

4 years, 10 months ago (2016-02-16 23:46:01 UTC) #25

boliu

On 2016/02/16 21:55:07, Charlie Reis (catching up) wrote: > [+boliu] > > Bo, can you ...

4 years, 10 months ago (2016-02-16 23:46:02 UTC) #26

mnaganov (inactive)

On 2016/02/16 23:46:02, boliu wrote: > On 2016/02/16 21:55:07, Charlie Reis (catching up) wrote: > ...

4 years, 10 months ago (2016-02-16 23:57:58 UTC) #27

Charlie Harrison

On 2016/02/16 23:57:58, mnaganov wrote: > On 2016/02/16 23:46:02, boliu wrote: > > On 2016/02/16 ...

4 years, 10 months ago (2016-02-17 00:30:21 UTC) #28

boliu

On 2016/02/17 00:30:21, csharrison wrote: > On 2016/02/16 23:57:58, mnaganov wrote: > > On 2016/02/16 ...

4 years, 10 months ago (2016-02-17 00:48:00 UTC) #29

On 2016/02/17 00:30:21, csharrison wrote:
> On 2016/02/16 23:57:58, mnaganov wrote:
> > On 2016/02/16 23:46:02, boliu wrote:
> > > On 2016/02/16 21:55:07, Charlie Reis (catching up) wrote:
> > > > [+boliu]
> > > > 
> > > > Bo, can you take a look at the change to
> > > > RenderFrameImpl::didStartProvisionalLoad?  We would pass the data URL
> > instead
> > > of
> > > > the base URL from WebContentsObserver::DidStartProvisionalLoad for
> > > > LoadDataWithBaseURL calls.  The tests seem to be happy but I'm not sure
if
> > > there
> > > > are compatibility implications.
> > > > 
> > > 
> > > Webview doesn't use DidStartProvisionalLoad callback for anything I think,
> so
> > > probably safe there.
> > > 
> > > I foresee one other problem though. What if the data URL is too big to be
> sent
> > > back as a GURL, which definitely does happen in webview. I think the IPC
> > system
> > > just drops that IPC in this case. Not sure if this is an existing problem
> with
> > > other IPCs already..
> > > 
> > > +mnaganov too
> > 
> > Yes, any attempt to serialize a huge GURL will result in a failure to send
an
> > IPC message, and the dialogue between renderer and browser will stuck.
That's
> > why WebView only allows huge data URLs in loadDataWithBaseURL, where the
> actual
> > data URL is only sent from the browser to renderer once, and for all other
> > messages either base or history URL (I forgot which of them) is used.
> > 
> > I don't actually know for sure, whether provisional load logic is used with
> > loadDataWithBaseURL. As we are not loading from network, maybe not?
> 
> Thanks for the clarification. For the record, we send the entire data: url in
> didCommitProvisionalLoad (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...)
> 
> We probably also use the provisional load data to display the url in the
> omnibox.
> 
> How big a GURL do we allow? I can test if things break.

IPC limits GURL to 2MB. Webview wants to support data URLs up to 20MB. mnaganov
added a special android-only path for loadDataWithBaseURL here:
https://codereview.chromium.org/1497743005. Keyword, android-only..

> 
> Also: I got the chromeos bug reproing locally, so hopefully I'll be able to
> track down that issue soon.

Charlie Reis

On 2016/02/17 00:48:00, boliu wrote: > On 2016/02/17 00:30:21, csharrison wrote: > > On 2016/02/16 ...

4 years, 10 months ago (2016-02-17 05:29:05 UTC) #30

On 2016/02/17 00:48:00, boliu wrote:
> On 2016/02/17 00:30:21, csharrison wrote:
> > On 2016/02/16 23:57:58, mnaganov wrote:
> > > On 2016/02/16 23:46:02, boliu wrote:
> > > > On 2016/02/16 21:55:07, Charlie Reis (catching up) wrote:
> > > > > [+boliu]
> > > > > 
> > > > > Bo, can you take a look at the change to
> > > > > RenderFrameImpl::didStartProvisionalLoad?  We would pass the data URL
> > > instead
> > > > of
> > > > > the base URL from WebContentsObserver::DidStartProvisionalLoad for
> > > > > LoadDataWithBaseURL calls.  The tests seem to be happy but I'm not
sure
> if
> > > > there
> > > > > are compatibility implications.
> > > > > 
> > > > 
> > > > Webview doesn't use DidStartProvisionalLoad callback for anything I
think,
> > so
> > > > probably safe there.
> > > > 
> > > > I foresee one other problem though. What if the data URL is too big to
be
> > sent
> > > > back as a GURL, which definitely does happen in webview. I think the IPC
> > > system
> > > > just drops that IPC in this case. Not sure if this is an existing
problem
> > with
> > > > other IPCs already..
> > > > 
> > > > +mnaganov too
> > > 
> > > Yes, any attempt to serialize a huge GURL will result in a failure to send
> an
> > > IPC message, and the dialogue between renderer and browser will stuck.
> That's
> > > why WebView only allows huge data URLs in loadDataWithBaseURL, where the
> > actual
> > > data URL is only sent from the browser to renderer once, and for all other
> > > messages either base or history URL (I forgot which of them) is used.
> > > 
> > > I don't actually know for sure, whether provisional load logic is used
with
> > > loadDataWithBaseURL. As we are not loading from network, maybe not?
> > 
> > Thanks for the clarification. For the record, we send the entire data: url
in
> > didCommitProvisionalLoad (see
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...)
> > 
> > We probably also use the provisional load data to display the url in the
> > omnibox.
> > 
> > How big a GURL do we allow? I can test if things break.
> 
> IPC limits GURL to 2MB. Webview wants to support data URLs up to 20MB.
mnaganov
> added a special android-only path for loadDataWithBaseURL here:
> https://codereview.chromium.org/1497743005. Keyword, android-only..
> 
> > 
> > Also: I got the chromeos bug reproing locally, so hopefully I'll be able to
> > track down that issue soon.

This is actually a good point-- I don't think it's safe to pass the data URL
here, for that reason.  We don't want to pass the entire thing back, even if IPC
supported it.  Is there a way to make an exception for this case on the other
side, making base URL the thing we expect in general for LoadDataWithBaseURL?

Charlie Harrison

On 2016/02/17 05:29:05, Charlie Reis wrote: > On 2016/02/17 00:48:00, boliu wrote: > > On ...

4 years, 10 months ago (2016-02-17 21:33:34 UTC) #31

On 2016/02/17 05:29:05, Charlie Reis wrote:
> On 2016/02/17 00:48:00, boliu wrote:
> > On 2016/02/17 00:30:21, csharrison wrote:
> > > On 2016/02/16 23:57:58, mnaganov wrote:
> > > > On 2016/02/16 23:46:02, boliu wrote:
> > > > > On 2016/02/16 21:55:07, Charlie Reis (catching up) wrote:
> > > > > > [+boliu]
> > > > > > 
> > > > > > Bo, can you take a look at the change to
> > > > > > RenderFrameImpl::didStartProvisionalLoad?  We would pass the data
URL
> > > > instead
> > > > > of
> > > > > > the base URL from WebContentsObserver::DidStartProvisionalLoad for
> > > > > > LoadDataWithBaseURL calls.  The tests seem to be happy but I'm not
> sure
> > if
> > > > > there
> > > > > > are compatibility implications.
> > > > > > 
> > > > > 
> > > > > Webview doesn't use DidStartProvisionalLoad callback for anything I
> think,
> > > so
> > > > > probably safe there.
> > > > > 
> > > > > I foresee one other problem though. What if the data URL is too big to
> be
> > > sent
> > > > > back as a GURL, which definitely does happen in webview. I think the
IPC
> > > > system
> > > > > just drops that IPC in this case. Not sure if this is an existing
> problem
> > > with
> > > > > other IPCs already..
> > > > > 
> > > > > +mnaganov too
> > > > 
> > > > Yes, any attempt to serialize a huge GURL will result in a failure to
send
> > an
> > > > IPC message, and the dialogue between renderer and browser will stuck.
> > That's
> > > > why WebView only allows huge data URLs in loadDataWithBaseURL, where the
> > > actual
> > > > data URL is only sent from the browser to renderer once, and for all
other
> > > > messages either base or history URL (I forgot which of them) is used.
> > > > 
> > > > I don't actually know for sure, whether provisional load logic is used
> with
> > > > loadDataWithBaseURL. As we are not loading from network, maybe not?
> > > 
> > > Thanks for the clarification. For the record, we send the entire data: url
> in
> > > didCommitProvisionalLoad (see
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...)
> > > 
> > > We probably also use the provisional load data to display the url in the
> > > omnibox.
> > > 
> > > How big a GURL do we allow? I can test if things break.
> > 
> > IPC limits GURL to 2MB. Webview wants to support data URLs up to 20MB.
> mnaganov
> > added a special android-only path for loadDataWithBaseURL here:
> > https://codereview.chromium.org/1497743005. Keyword, android-only..
> > 
> > > 
> > > Also: I got the chromeos bug reproing locally, so hopefully I'll be able
to
> > > track down that issue soon.
> 
> This is actually a good point-- I don't think it's safe to pass the data URL
> here, for that reason.  We don't want to pass the entire thing back, even if
IPC
> supported it.  Is there a way to make an exception for this case on the other
> side, making base URL the thing we expect in general for LoadDataWithBaseURL?

I agree, but it's not looking trivial to change behavior so all
LoadDataWithBaseURL loads send the history or base url. Since the pending entry
is initialized on the browser side, we need to make sure we *dont* change it on
commit (I think this happens near RendererDidNavigate). This way the browser
always has the "correct" url (the data url) in the entry, but we never send it
over IPC. The "original url" will also have to be changed I believe.

This is probably enough work for a separate CL. Would you be opposed to landing
the approach in Patch set 9, where we thread the entry ids for this data url
case? That way we don't modify the data url behavior but we can land this first.

Charlie Reis

On 2016/02/17 21:33:34, csharrison wrote: > On 2016/02/17 05:29:05, Charlie Reis wrote: > > On ...

4 years, 10 months ago (2016-02-19 18:00:12 UTC) #32

On 2016/02/17 21:33:34, csharrison wrote:
> On 2016/02/17 05:29:05, Charlie Reis wrote:
> > On 2016/02/17 00:48:00, boliu wrote:
> > > On 2016/02/17 00:30:21, csharrison wrote:
> > > > On 2016/02/16 23:57:58, mnaganov wrote:
> > > > > On 2016/02/16 23:46:02, boliu wrote:
> > > > > > On 2016/02/16 21:55:07, Charlie Reis (catching up) wrote:
> > > > > > > [+boliu]
> > > > > > > 
> > > > > > > Bo, can you take a look at the change to
> > > > > > > RenderFrameImpl::didStartProvisionalLoad?  We would pass the data
> URL
> > > > > instead
> > > > > > of
> > > > > > > the base URL from WebContentsObserver::DidStartProvisionalLoad for
> > > > > > > LoadDataWithBaseURL calls.  The tests seem to be happy but I'm not
> > sure
> > > if
> > > > > > there
> > > > > > > are compatibility implications.
> > > > > > > 
> > > > > > 
> > > > > > Webview doesn't use DidStartProvisionalLoad callback for anything I
> > think,
> > > > so
> > > > > > probably safe there.
> > > > > > 
> > > > > > I foresee one other problem though. What if the data URL is too big
to
> > be
> > > > sent
> > > > > > back as a GURL, which definitely does happen in webview. I think the
> IPC
> > > > > system
> > > > > > just drops that IPC in this case. Not sure if this is an existing
> > problem
> > > > with
> > > > > > other IPCs already..
> > > > > > 
> > > > > > +mnaganov too
> > > > > 
> > > > > Yes, any attempt to serialize a huge GURL will result in a failure to
> send
> > > an
> > > > > IPC message, and the dialogue between renderer and browser will stuck.
> > > That's
> > > > > why WebView only allows huge data URLs in loadDataWithBaseURL, where
the
> > > > actual
> > > > > data URL is only sent from the browser to renderer once, and for all
> other
> > > > > messages either base or history URL (I forgot which of them) is used.
> > > > > 
> > > > > I don't actually know for sure, whether provisional load logic is used
> > with
> > > > > loadDataWithBaseURL. As we are not loading from network, maybe not?
> > > > 
> > > > Thanks for the clarification. For the record, we send the entire data:
url
> > in
> > > > didCommitProvisionalLoad (see
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...)
> > > > 
> > > > We probably also use the provisional load data to display the url in the
> > > > omnibox.
> > > > 
> > > > How big a GURL do we allow? I can test if things break.
> > > 
> > > IPC limits GURL to 2MB. Webview wants to support data URLs up to 20MB.
> > mnaganov
> > > added a special android-only path for loadDataWithBaseURL here:
> > > https://codereview.chromium.org/1497743005. Keyword, android-only..
> > > 
> > > > 
> > > > Also: I got the chromeos bug reproing locally, so hopefully I'll be able
> to
> > > > track down that issue soon.
> > 
> > This is actually a good point-- I don't think it's safe to pass the data URL
> > here, for that reason.  We don't want to pass the entire thing back, even if
> IPC
> > supported it.  Is there a way to make an exception for this case on the
other
> > side, making base URL the thing we expect in general for
LoadDataWithBaseURL?
> 
> I agree, but it's not looking trivial to change behavior so all
> LoadDataWithBaseURL loads send the history or base url. Since the pending
entry
> is initialized on the browser side, we need to make sure we *dont* change it
on
> commit (I think this happens near RendererDidNavigate). This way the browser
> always has the "correct" url (the data url) in the entry, but we never send it
> over IPC. The "original url" will also have to be changed I believe.
> 
> This is probably enough work for a separate CL. Would you be opposed to
landing
> the approach in Patch set 9, where we thread the entry ids for this data url
> case? That way we don't modify the data url behavior but we can land this
first.

Yes, I'm totally fine with a workaround for now and revisiting it later.  Patch
Set 9 looks good with a few comments and questions.

https://codereview.chromium.org/1661423002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1661423002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1008: // TODO(csharrison):
Data navigations with base URLs get reset here, because
Mention LoadDataWithBaseURL explicitly here.

https://codereview.chromium.org/1661423002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1009: // the
NavigationHandle tracks the base URL but the validated_params.url track
nit: tracks the data

https://codereview.chromium.org/1661423002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1011: // should go away
when this is properly handled.
Let's add a bug number for this work, since it's going to require a
discussion/investigation of its own.

https://codereview.chromium.org/1661423002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1015: if
(navigation_handle_->GetURL() == validated_params.base_url)
This base_url gets used for other cases as well, according to
FrameNavigateParams.  I'm a bit concerned that this could make us propagate the
entry ID for a non-matching renderer-initiated synchronous nav (e.g.,
pushState), if it can have a base_url that matches the pending NavigationHandle.

Is there any way to check something that's specific to LoadDataWithBaseURL?  I
see GetBaseURLForDataURL on NavigationEntry, though it's not great to look at
NavigationEntries directly here (and I'm not 100% sure if it will always exist).
 NavigationHandle doesn't seem to track it yet, though, so maybe the entry would
do.

https://codereview.chromium.org/1661423002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1025: // nav_id. If the
previous handle was a prematurely aborted data nav, then
Note: It's important to distinguish between a data URL navigation (which is
commonplace and doesn't break any rules) and LoadDataWithBaseURL (which is
Android Webview specific and causes problems like this).

Charlie Harrison

PTAL. I had to change the chromeos test. I think our misbehaving logic was covering ...

4 years, 10 months ago (2016-02-22 14:50:42 UTC) #33

Charlie Harrison

csharrison@chromium.org changed reviewers: + bauerb@chromium.org

4 years, 10 months ago (2016-02-24 17:32:12 UTC) #34

Bernhard Bauer

https://codereview.chromium.org/1661423002/diff/280001/chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc File chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc (right): https://codereview.chromium.org/1661423002/diff/280001/chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc#newcode143 chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:143: GURL("chrome://settings-frame/contentExceptions#media-stream-camera"); Why do we not do this for microphone?

4 years, 10 months ago (2016-02-25 09:32:43 UTC) #36

Charlie Harrison

https://codereview.chromium.org/1661423002/diff/280001/chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc File chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc (right): https://codereview.chromium.org/1661423002/diff/280001/chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc#newcode143 chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:143: GURL("chrome://settings-frame/contentExceptions#media-stream-camera"); On 2016/02/25 at 09:32:43, Bernhard Bauer wrote: > ...

4 years, 10 months ago (2016-02-25 13:24:17 UTC) #37

Bernhard Bauer

https://codereview.chromium.org/1661423002/diff/280001/chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc File chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc (right): https://codereview.chromium.org/1661423002/diff/280001/chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc#newcode143 chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:143: GURL("chrome://settings-frame/contentExceptions#media-stream-camera"); On 2016/02/25 13:24:17, csharrison wrote: > On 2016/02/25 ...

4 years, 10 months ago (2016-02-25 13:47:07 UTC) #38

Charlie Reis

content/ LGTM with one question. https://codereview.chromium.org/1661423002/diff/280001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1661423002/diff/280001/content/browser/frame_host/render_frame_host_impl.cc#newcode1036 content/browser/frame_host/render_frame_host_impl.cc:1036: frame_tree_node()->navigator()->GetController()->GetPendingEntry(); I take it ...

4 years, 10 months ago (2016-02-25 22:22:33 UTC) #39

clamy

Thanks! A few questions. https://codereview.chromium.org/1661423002/diff/280001/content/browser/frame_host/navigator_impl.cc File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1661423002/diff/280001/content/browser/frame_host/navigator_impl.cc#newcode159 content/browser/frame_host/navigator_impl.cc:159: navigation_start, pending_entry ? pending_entry->GetUniqueID() : ...

4 years, 10 months ago (2016-02-26 14:08:13 UTC) #40

clamy

I'm also having your patch run through the PlzNavigate trybot (note: it will return red ...

4 years, 10 months ago (2016-02-26 14:25:45 UTC) #41

Charlie Harrison

I think my changes ensure that subframes will get 0 as a pending_nav_entry_id. https://codereview.chromium.org/1661423002/diff/280001/content/browser/frame_host/navigator_impl.cc File ...

4 years, 10 months ago (2016-02-26 17:39:51 UTC) #42

I think my changes ensure that subframes will get 0 as a pending_nav_entry_id.

https://codereview.chromium.org/1661423002/diff/280001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1661423002/diff/280001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:159: navigation_start,
pending_entry ? pending_entry->GetUniqueID() : 0));
On 2016/02/26 at 14:08:13, clamy wrote:
> Can the pending entry ever be null here except in the error page case?

It can be null for subframes. Good catch.

https://codereview.chromium.org/1661423002/diff/280001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:835:
DCHECK(controller_->GetPendingEntry());
On 2016/02/26 at 14:08:13, clamy wrote:
> Is it always the case with subframes? Subframes will go through this codepath
as well.

You've caught an error. I don't believe that subframes will have a pending
entry.

https://codereview.chromium.org/1661423002/diff/280001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1661423002/diff/280001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1036:
frame_tree_node()->navigator()->GetController()->GetPendingEntry();
On 2016/02/25 at 22:22:33, Charlie Reis (slow til Mar 7) wrote:
> I take it there's a pending entry in these cases?  If so, looking at it should
work for now, until we can fix crbug.com/588317.

I believe there will always be a pending entry for these data urls, which will
always be main frame.

https://codereview.chromium.org/1661423002/diff/280001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1043:
!pending_entry->GetBaseURLForDataURL().is_empty()) {
On 2016/02/25 at 22:22:33, Charlie Reis (slow til Mar 7) wrote:
> Should this be checking if pending_entry->GetBaseURLForDataURL() is equal to
validated_params.base_url?

Yep. Done.

Charlie Harrison

On 2016/02/29 16:41:39, clamy wrote: > Thanks! PlzNavigate part lgtm. Charlie, there was some difficulty ...

4 years, 9 months ago (2016-02-29 19:19:17 UTC) #45

Charlie Harrison

Landing this for now as the checks in RenderFrameHostImpl are sufficient to match data urls ...

4 years, 9 months ago (2016-03-01 14:25:13 UTC) #46

Charlie Harrison

The CQ bit was checked by csharrison@chromium.org

4 years, 9 months ago (2016-03-01 14:25:49 UTC) #47

Charlie Harrison

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, bauerb@chromium.org Link to ...

4 years, 9 months ago (2016-03-01 14:25:50 UTC) #48

commit-bot: I haz the power

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

4 years, 9 months ago (2016-03-01 14:26:08 UTC) #49

commit-bot: I haz the power

Description was changed from ========== This patch gives the NavigationHandle a NavigationEntry on instantiation, so ...

4 years, 9 months ago (2016-03-01 17:24:14 UTC) #50

commit-bot: I haz the power

Description was changed from ========== This patch gives the NavigationHandle a NavigationEntry on instantiation, so ...

4 years, 9 months ago (2016-03-01 17:25:08 UTC) #52

commit-bot: I haz the power

Patchset 17 (id:??) landed as https://crrev.com/9a9142bc4afafb16c2245ec30d018b0089e2d20f Cr-Commit-Position: refs/heads/master@{#378463}

4 years, 9 months ago (2016-03-01 17:25:09 UTC) #53

dschuyler

A revert of this CL (patchset #17 id:320001) has been created in https://codereview.chromium.org/1755733002/ by dschuyler@chromium.org. ...

4 years, 9 months ago (2016-03-01 18:01:06 UTC) #54

Message was sent while issue was closed.

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/1755733002/ by dschuyler@chromium.org.

The reason for reverting is: The android tree auto closed.  I don't see which CL
is causing the trouble, so this is a guess.

Here are some logs from
https://build.chromium.org/p/chromium/builders/Android/builds/52518

[6697/26456] CXX obj/third_party/skia/src/effects/skia_library.SkPackBits.o
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
[6698/26456] STAMP obj/third_party/junit/junit_jar.actions_rules_copies.stamp
[6699/26456] STAMP obj/third_party/mockito/mockito_jar.actions_depends.stamp
[6700/26456] ACTION Compiling mockito_jar java sources
FAILED: cd ../../ui/accessibility; python ../../build/android/gyp/lint.py
"--lint-path=/b/build/slave/Android/build/src/third_party/android_tools/sdk//tools/lint"
"--config-path=../../build/android/lint/suppressions.xml"
"--processed-config-path=../../out/Release/gen/ui_accessibility_java/lint_config.xml"
"--manifest-path=../../build/android/AndroidManifest.xml"
"--result-path=../../out/Release/gen/ui_accessibility_java/lint_result.xml"
"--resource-dir=../../build/android/ant/empty/res"
"--product-dir=../../out/Release" "--src-dirs=../../build/android/empty/src"
"--jar-path=../../out/Release/lib.java/ui_accessibility_java.jar"
--can-fail-build
"--stamp=../../out/Release/gen/ui_accessibility_java/lint.stamp" --enable
Lint created unparseable xml file...
File contents:

Traceback (most recent call last):
  File "../../build/android/gyp/lint.py", line 231, in <module>
    sys.exit(main())
  File "../../build/android/gyp/lint.py", line 227, in main
    pass_changes=True)
  File "/b/build/slave/Android/build/src/build/android/gyp/util/build_utils.py",
line 513, in CallAndWriteDepfileIfStale
    pass_changes=True)
  File "/b/build/slave/Android/build/src/build/android/gyp/util/md5_check.py",
line 87, in CallAndRecordIfStale
    function(*args)
  File "/b/build/slave/Android/build/src/build/android/gyp/util/build_utils.py",
line 500, in on_stale_md5
    function(*args)
  File "../../build/android/gyp/lint.py", line 222, in <lambda>
    can_fail_build=options.can_fail_build),
  File "../../build/android/gyp/lint.py", line 136, in _OnStaleMd5
    num_issues = _ParseAndShowResultFile()
  File "../../build/android/gyp/lint.py", line 54, in _ParseAndShowResultFile
    dom = minidom.parse(result_path)
  File "/usr/lib/python2.7/xml/dom/minidom.py", line 1920, in parse
    return expatbuilder.parse(file)
  File "/usr/lib/python2.7/xml/dom/expatbuilder.py", line 924, in parse
    result = builder.parseFile(fp)
  File "/usr/lib/python2.7/xml/dom/expatbuilder.py", line 211, in parseFile
    parser.Parse("", True)
xml.parsers.expat.ExpatError: no element found: line 1, column 0
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
ninja: build stopped: subcommand failed.
.

Charlie Harrison

4 years, 9 months ago (2016-03-03 21:35:41 UTC) #55

Message was sent while issue was closed.

On 2016/03/01 at 18:01:06, dschuyler wrote:
> A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/1755733002/ by dschuyler@chromium.org.
> 
> The reason for reverting is: The android tree auto closed.  I don't see which
CL
> is causing the trouble, so this is a guess.
> 
> Here are some logs from
https://build.chromium.org/p/chromium/builders/Android/builds/52518
> 
> [6697/26456] CXX obj/third_party/skia/src/effects/skia_library.SkPackBits.o
> Note: Some input files use unchecked or unsafe operations.
> Note: Recompile with -Xlint:unchecked for details.
> [6698/26456] STAMP obj/third_party/junit/junit_jar.actions_rules_copies.stamp
> [6699/26456] STAMP obj/third_party/mockito/mockito_jar.actions_depends.stamp
> [6700/26456] ACTION Compiling mockito_jar java sources
> FAILED: cd ../../ui/accessibility; python ../../build/android/gyp/lint.py
"--lint-path=/b/build/slave/Android/build/src/third_party/android_tools/sdk//tools/lint"
"--config-path=../../build/android/lint/suppressions.xml"
"--processed-config-path=../../out/Release/gen/ui_accessibility_java/lint_config.xml"
"--manifest-path=../../build/android/AndroidManifest.xml"
"--result-path=../../out/Release/gen/ui_accessibility_java/lint_result.xml"
"--resource-dir=../../build/android/ant/empty/res"
"--product-dir=../../out/Release" "--src-dirs=../../build/android/empty/src"
"--jar-path=../../out/Release/lib.java/ui_accessibility_java.jar"
--can-fail-build
"--stamp=../../out/Release/gen/ui_accessibility_java/lint.stamp" --enable
> Lint created unparseable xml file...
> File contents:
> 
> Traceback (most recent call last):
>   File "../../build/android/gyp/lint.py", line 231, in <module>
>     sys.exit(main())
>   File "../../build/android/gyp/lint.py", line 227, in main
>     pass_changes=True)
>   File
"/b/build/slave/Android/build/src/build/android/gyp/util/build_utils.py", line
513, in CallAndWriteDepfileIfStale
>     pass_changes=True)
>   File "/b/build/slave/Android/build/src/build/android/gyp/util/md5_check.py",
line 87, in CallAndRecordIfStale
>     function(*args)
>   File
"/b/build/slave/Android/build/src/build/android/gyp/util/build_utils.py", line
500, in on_stale_md5
>     function(*args)
>   File "../../build/android/gyp/lint.py", line 222, in <lambda>
>     can_fail_build=options.can_fail_build),
>   File "../../build/android/gyp/lint.py", line 136, in _OnStaleMd5
>     num_issues = _ParseAndShowResultFile()
>   File "../../build/android/gyp/lint.py", line 54, in _ParseAndShowResultFile
>     dom = minidom.parse(result_path)
>   File "/usr/lib/python2.7/xml/dom/minidom.py", line 1920, in parse
>     return expatbuilder.parse(file)
>   File "/usr/lib/python2.7/xml/dom/expatbuilder.py", line 924, in parse
>     result = builder.parseFile(fp)
>   File "/usr/lib/python2.7/xml/dom/expatbuilder.py", line 211, in parseFile
>     parser.Parse("", True)
> xml.parsers.expat.ExpatError: no element found: line 1, column 0
> Note: Some input files use or override a deprecated API.
> Note: Recompile with -Xlint:deprecation for details.
> Note: Some input files use unchecked or unsafe operations.
> Note: Recompile with -Xlint:unchecked for details.
> ninja: build stopped: subcommand failed.
> .

FYI this change was not reverted, as the tree closure was a flake.

Issue 1661423002: Solidify Entry discarding logic (NavigationHandle keeps its ID) (Closed)

Description

Patch Set 1 #

Patch Set 2 : null checks #

Patch Set 3 : Added unittest #

Patch Set 4 : creis review #

Patch Set 5 : Addressed creis@ comments #

Patch Set 6 : Self review: remove includes and improve a comment #

Patch Set 7 : #

Patch Set 8 : nav id hack for data navs with base url #

Patch Set 9 : rebase + creis review #

Patch Set 10 : Don't initialize NavigationHandle with base url for data url #

Patch Set 11 : creis@ review #

Patch Set 12 : back to patch set 9 + paranoia #

Patch Set 13 : Patch test that surfaced a bug in chromeos #

Patch Set 14 : fixed typo #

Patch Set 15 : merge @377292 #

Patch Set 16 : fix plznavigate (I hope), compare base_urls, re-order ManageLink test #

Patch Set 17 : Fix check that's invalid when base url has invalid url. Added TODO #

Messages