Issue metadata
Sign in to add a comment
|
loadDataWithBaseUrl calls shouldInterceptRequest for data URL |
||||||||||||||||||||||
Issue descriptionFollow-up to issue 702785 . This is a bug introduced by PlzNavigate. I'll investigate similar APIs (loadData, loadUrl) and figure out the best fix.
,
Oct 3 2017
,
Oct 3 2017
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..
,
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
,
Oct 3 2017
and finally, the tests: https://chromium-review.googlesource.com/#/c/chromium/src/+/698724
,
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
,
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.
,
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)
,
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
,
Oct 4 2017
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.
,
Oct 4 2017
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.
,
Oct 4 2017
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..
,
Oct 4 2017
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?
,
Oct 4 2017
let's focus on plznavigate issues first
,
Oct 4 2017
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.
,
Oct 4 2017
I suppose there's some risk with hack too, in case any app actually uses an empty data url for something..
,
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
,
Oct 5 2017
remove dependency and set a next action date after m63 stable
,
Dec 11 2017
The NextAction date has arrived: 2017-12-11
,
Dec 11 2017
m64 just branched. now's a pretty good time to remove the hack, and cross our fingers
,
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
,
Dec 12 2017
Test being disabled because it doesn't pass with old navigation path. Keeping this open until test can be enabled.
,
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
,
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...
,
Mar 8 2018
,
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
,
Mar 15 2018
Closing this, because I enabled the test, as per #22
,
Mar 20 2018
Please add the manual verification steps to verify this issue.
,
Mar 20 2018
There is no need for manual verification here. Change captured in a test already. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by nasko@chromium.org
, Oct 3 2017