New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 769126 link

Starred by 5 users

Issue metadata

Status: Verified
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: 2017-12-11
OS: Android
Pri: 2
Type: Bug

Blocking:
issue 766255



Sign in to add a comment

loadDataWithBaseUrl calls shouldInterceptRequest for data URL

Project Member Reported by ntfschr@chromium.org, Sep 27 2017

Issue description

Follow-up to  issue 702785 .

This is a bug introduced by PlzNavigate.

I'll investigate similar APIs (loadData, loadUrl) and figure out the best fix.
 

Comment 1 by nasko@chromium.org, Oct 3 2017

With PlzNavigate, navigations to data: URLs are using NetworkRequest and are being sent to the network stack. The reason is that data: URLs contain mime type, which can result in a download and the mime parser/download logic is all implemented at the network layer.

It looks like WebView code is now getting notified about these requests and it might be just a matter of ignoring the requests for data: URLs, which will get it back to the original behavior.

Comment 2 by boliu@chromium.org, Oct 3 2017

Cc: ntfschr@chromium.org
Owner: boliu@chromium.org

Comment 3 by boliu@chromium.org, Oct 3 2017

Cc: nasko@chromium.org
uh oh, this appears more complicated than originally thought. with plznavigate disabled:

loadDataWithBaseUrl - no shouldInterceptRequest called

loadData - shouldInterceptRequest called with data url constructed here: https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chromium/content_public/browser/LoadUrlParams.java?rcl=c5f8071c7ed53ddf846bd886f87809dca14583ab&l=102

loadUrl("data:...") - shouldInterceptRequest called with data url

CL of tests being uploaded, but taking a minute..

Comment 4 by boliu@chromium.org, Oct 3 2017

good thing is loadData and loadUrl cases already pass when plznavigate is turned on. so maybe there's special code somewhere to insert these in webview somewhere. I haven't looked. So maybe original idea of "filter out data urls from content" is still ok. I'll have to check

Comment 6 by boliu@chromium.org, Oct 3 2017

There are no special code paths in webview. Content was going through network stack for data loadData and loadUrl before. It's just loadDataWithBaseUrl was special.

And we should drop the shouldInterceptRequest for loadDataWithBaseUrl. The url passed up to shouldInterceptRequest is an empty data url (because the data is stored in a string, not in a GURL).

So then... a hacky fix would be to ignore empty data urls.. but oh so hacky. Proper change will have to involve changes in content, presumably

Comment 7 by nasko@chromium.org, Oct 3 2017

What would "proper change" look like? I don't mind changing content/, assuming it is a reasonable change. loadDataWithBaseUrl has already been a hacky API with lots of weird implications, so not making it even more hacky is good. Also, finding simpler ways to do it or just removing it ;) will be even more awesome :D.

Comment 8 by boliu@chromium.org, Oct 3 2017

content knows whether a load is loadDataWithBaseUrl. so it basically needs to plumb that value through net::URLRequest, up to webview's URLRequestJob, so it knows to not send ShouldInterceptRequest for that request. URLRequest already supports user data so perhaps it's not too horrible

otoh, the hack fix passed all tests: https://chromium-review.googlesource.com/c/chromium/src/+/698724 (PS2)

Comment 9 by boliu@chromium.org, Oct 4 2017

So the stack (on IO thread) for this is:

  0009f3a5  android_webview::(anonymous namespace)::ShouldInterceptRequestAdaptor::ObtainDelegate(net::URLRequest*, base::RepeatingCallback<void (std::__ndk1::unique_ptr<android_webview::AndroidStreamReaderURLRequestJob::Delegate, std::__ndk1::default_delete<android_webview::AndroidStreamReaderURLRequestJob::Delegate> >)> const&)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                             /android/chromium/src/android_webview/browser/net/aw_request_interceptor.cc:92
  00099eaf  android_webview::AndroidStreamReaderURLRequestJob::Start()                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  /android/chromium/src/android_webview/browser/net/android_stream_reader_url_request_job.cc:140
  002f8fd7  net::URLRequest::StartJob(net::URLRequestJob*)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                              /android/chromium/src/net/url_request/url_request.cc:668
  002f8def  net::URLRequest::BeforeRequestComplete(int)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                 /android/chromium/src/net/url_request/url_request.cc:613
  002f8bd5  net::URLRequest::Start()                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                    /android/chromium/src/net/url_request/url_request.cc:538
  00859573  content::ResourceLoader::StartRequestInternal()                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                             /android/chromium/src/content/browser/loader/resource_loader.cc:594
  00859311  content::ResourceLoader::Resume(bool)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                       /android/chromium/src/content/browser/loader/resource_loader.cc:513
  00857d65  content::ResourceLoader::ScopedDeferral::~ScopedDeferral()                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  /android/chromium/src/content/browser/loader/resource_loader.cc:208
  00857c9b  content::ResourceLoader::StartRequest()                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                     /android/chromium/src/content/browser/loader/resource_loader.cc:254
  00855a53  content::ResourceDispatcherHostImpl::StartLoading(content::ResourceRequestInfoImpl*, std::__ndk1::unique_ptr<content::ResourceLoader, std::__ndk1::default_delete<content::ResourceLoader> >)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                               /android/chromium/src/content/browser/loader/resource_dispatcher_host_impl.cc:2405
  00854813  content::ResourceDispatcherHostImpl::BeginRequestInternal(std::__ndk1::unique_ptr<net::URLRequest, std::__ndk1::default_delete<net::URLRequest> >, std::__ndk1::unique_ptr<content::ResourceHandler, std::__ndk1::default_delete<content::ResourceHandler> >)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                               /android/chromium/src/content/browser/loader/resource_dispatcher_host_impl.cc:2312
  00855859  content::ResourceDispatcherHostImpl::BeginNavigationRequest(content::ResourceContext*, net::URLRequestContext*, storage::FileSystemContext*, content::NavigationRequestInfo const&, std::__ndk1::unique_ptr<content::NavigationUIData, std::__ndk1::default_delete<content::NavigationUIData> >, content::NavigationURLLoaderImplCore*, content::ServiceWorkerNavigationHandleCore*, content::AppCacheNavigationHandleCore*)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                /android/chromium/src/content/browser/loader/resource_dispatcher_host_impl.cc:2203


At the bottom NavigationURLLoaderImplCore has NavigationRequestInfo, which contains the information of whether that navigation is from a loadDataWithBaseURL or not. Then ResourceDispatcherHostImpl is the one the actually constructs the net::URLRequest, and I think it needs to set a userdata that indicates whether this request is a loadDataWithBaseURL, perhaps another parameter in URLRequestUserData? Then webview can read it in its code


nasko: does that sound reasaonble? If so, I can start working on a CL tomorrow
I'm afraid that as we move to the network service architecture, we can get back into the same problem. The more I think about it, the more appealing it is to take the hacky way of handling it inside WebView code itself.

Is there a pattern of already attaching WebView specific UserData? In general, I'd rather us not expose more details of loadDataWithBaseURL than we have to, as it is always a case fraught with fragility.
I imagine a lot of servification work isn't going to work well in webview, or at least, behind the scenes, webview will have to tie separate services back together again.

> Is there a pattern of already attaching WebView specific UserData?

no

> In general, I'd rather us not expose more details of loadDataWithBaseURL than we have to, as it is always a case fraught with fragility.

The hack fix is doing exactly that though. It's an implementation detail in content that loadDataWithBaseURL stores the data in a string rather than in the url field, and just leaves an empty data string in the url field instead. And the hack fix is just relying on the implementation that in theory could change at any point.

I imagine there isn't any way to tie a resource load back to a top level browser-initiated navigation request, so I imagine content has to do something to help here.
alternatively, I don't really understand why loadDataWithBaseUrl is special here... maybe can get away with landing the hack fix temporarily, but with the plan to actually just remove the hack and call shouldInterceptRequest for loadDataWithBaseUrl in the future, after plznavigate successfully ships, probably..
c#12 is mostly reasonable, but some awkwardness:

 1. onPageStarted gets the base URL, onPageFinished gets the base URL, but shouldInterceptRequest/onLoadResource don't?
 2. loadData triggers shouldInterceptRequest with the real data URL. loadDataWithBaseUrl triggers shouldInterceptRequest with an empty data URL.

Perhaps we could consider exposing the real data URL?
let's focus on plznavigate issues first
Ok, sounds like everyone is leaning towards the hack fix. I'll clean up the Cl and make sure it's only a hack that should be removed later.
I suppose there's some risk with hack too, in case any app actually uses an empty data url for something..
Project Member

Comment 17 by bugdroid1@chromium.org, Oct 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cb8eb9fd2cc8ccd221bffb32b1569a6c1c9a71b4

commit cb8eb9fd2cc8ccd221bffb32b1569a6c1c9a71b4
Author: Bo Liu <boliu@chromium.org>
Date: Wed Oct 04 23:47:05 2017

aw: shouldInterceptRequest for LoadDataWithBaseURL hack

Old behavior before PlzNavigate was that resource for a
LoadDataWithBaseURL navigation does *not* trigger
shouldInterceptRequest.

Also add tests to verify shouldInterceptRequest with various
load data calls.

This CL adds a hack to get back to the same behavior. Hack works
by filtering out empty data URLs, which through a quirk in content
is what's sent up for the shouldInterceptRequest call.

This is a hack because it's relying on a content implementation
detail which can change at any time. It is also risky if there are
legitimate cases where an empty data URL is used in practice.

This is short term fix to ship PlzNavigate in webview, but this
hack should definitely be revisited soon after.

Bug:  769126 
Change-Id: I70b5ca44a33126f6470978a9ffe8f7d8126c15bd
Reviewed-on: https://chromium-review.googlesource.com/698724
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506569}
[modify] https://crrev.com/cb8eb9fd2cc8ccd221bffb32b1569a6c1c9a71b4/android_webview/browser/aw_contents_io_thread_client.cc
[modify] https://crrev.com/cb8eb9fd2cc8ccd221bffb32b1569a6c1c9a71b4/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java

Blocking: -766255
NextAction: 2017-12-11
remove dependency and set a next action date after m63 stable
The NextAction date has arrived: 2017-12-11

Comment 20 by boliu@chromium.org, Dec 11 2017

m64 just branched. now's a pretty good time to remove the hack, and cross our fingers
Project Member

Comment 21 by bugdroid1@chromium.org, Dec 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/06d9a1d7fc69fe188f0c8af467fd66519cc354bb

commit 06d9a1d7fc69fe188f0c8af467fd66519cc354bb
Author: Bo Liu <boliu@chromium.org>
Date: Tue Dec 12 03:45:36 2017

aw: Call shouldInterceptRequest for LoadDataWithBaseURL

This is a behavior change. Call shouldInterceptRequest for
LoadDataWithBaseURL calls. The other two loadData calls both trigger
shouldInterceptRequest. Remove the hack that checks for
loadDataWithBaseURL, which then starts triggering
shouldInterceptRequests as well.

Bug:  769126 
Change-Id: Ia6994186bab8cb4647297d49bab6e662b47164af
Reviewed-on: https://chromium-review.googlesource.com/820717
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523317}
[modify] https://crrev.com/06d9a1d7fc69fe188f0c8af467fd66519cc354bb/android_webview/browser/aw_contents_io_thread_client.cc
[modify] https://crrev.com/06d9a1d7fc69fe188f0c8af467fd66519cc354bb/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java

Comment 22 by boliu@chromium.org, Dec 12 2017

Test being disabled because it doesn't pass with old navigation path. Keeping 
this open until test can be enabled.
Project Member

Comment 23 by bugdroid1@chromium.org, Dec 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/13812c6d31fbfe3ff2cb385a7b4114ffd7fd9bf2

commit 13812c6d31fbfe3ff2cb385a7b4114ffd7fd9bf2
Author: Bo Liu <boliu@chromium.org>
Date: Tue Dec 12 18:18:17 2017

aw: Disable test failing on renderer navigation mode

Turns out the old navigation mode is still being tested, which of course
fails the test. Disable it for now.

TBR=ntfschr@chromium.org

Bug:  794117 ,  769126 
Change-Id: I845d856cbdf8e4319c4363810e88e82b9ed40488
Reviewed-on: https://chromium-review.googlesource.com/822757
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523478}
[modify] https://crrev.com/13812c6d31fbfe3ff2cb385a7b4114ffd7fd9bf2/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java

Comment 24 by boliu@chromium.org, Dec 15 2017

b/68992755 is a long investigation with gmail, where part of the conclusion is that if loadDataWithBaseURL skipped shouldInterceptRequest *and all the work associated with it on the blocking pool*, then it has the potential to significantly speed up gmail loads

hmm...
Blocking: 766255
Project Member

Comment 26 by bugdroid1@chromium.org, Mar 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6ba50de74bbbed1ccda6e850b0698cfec172690a

commit 6ba50de74bbbed1ccda6e850b0698cfec172690a
Author: Nate Fischer <ntfschr@chromium.org>
Date: Thu Mar 15 17:48:49 2018

AW: enable PlzNavigate-specific test

This was originally disabled because we still ran tests against the old
code path. That's no longer supported, so we can safely reenable the
test.

This also turns a comment into a proper TODO, since it shouldn't be left
too long.

Bug:  822124 
Bug:  769126 
Test: run_webview_instrumentation_test_apk -f AwContentsClientShouldInterceptRequestTest#testLoadDataWithBaseUrlTriggersShouldInterceptRequest
Change-Id: I0947560b44c565edb3be2966e121d916b003ed01
Reviewed-on: https://chromium-review.googlesource.com/963647
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543434}
[modify] https://crrev.com/6ba50de74bbbed1ccda6e850b0698cfec172690a/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java

Status: Fixed (was: Assigned)
Closing this, because I enabled the test, as per #22
Please add the manual verification steps to verify this issue.

Comment 29 by boliu@chromium.org, Mar 20 2018

Status: Verified (was: Fixed)
There is no need for manual verification here. Change captured in a test already.

Sign in to add a comment