Base URL is not restored by NavigationController when a new NavigationEntry is created in RendererDidNavigateToNewPage |
||||||||||
Issue descriptionBasically, what the summary says. See also b/27534809 for details. In Android WebView, when `loadDataFromBaseURL` is called alone, at the moment when NavigationController receives `RendererDidNavigateToNewPage` there is a pending entry, and the current entry gets cloned from it, preserving the base URL. But when `loadDataFromBaseURL` is followed by `loadUrl("javascript:...")`, the pending entry is not used, and a new navigation entry gets created instead containing only the data URL received from the renderer. We just need to set base URL when creating a new entry. We actually have base URL in the IPC message params.
,
Mar 11 2016
I agree. I would even merge it into M49 if possible.
,
Mar 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ce1f4d009316d8f5f4c49b18e1bd94687d80c59d commit ce1f4d009316d8f5f4c49b18e1bd94687d80c59d Author: mnaganov <mnaganov@chromium.org> Date: Mon Mar 14 23:24:54 2016 Store and use last base URL between DidStart / DidStopLoading As it turns out that the committed entry may omit the base URL if there has been a javascript: URL navigation after LoadDataWithBaseURL. Thus we have to store the base url on DidStartLoading and use it in DidStopLoading / DidFinishLoad. We have to preserve the old hack with SetToBaseURLForDataURLIfNeeded because if the base URL is not valid, DidFinishLoad doesn't receive it from the renderer. This change is a short-term workaround. The correct solution is to pass a flag that the load was through LoadDataWithBaseURL via Blink, so base URL can be restored correctly within NavigationController. BUG= 594001 Review URL: https://codereview.chromium.org/1779363004 Cr-Commit-Position: refs/heads/master@{#381108} [modify] https://crrev.com/ce1f4d009316d8f5f4c49b18e1bd94687d80c59d/android_webview/javatests/src/org/chromium/android_webview/test/LoadDataWithBaseUrlTest.java [modify] https://crrev.com/ce1f4d009316d8f5f4c49b18e1bd94687d80c59d/content/browser/android/web_contents_observer_proxy.cc [modify] https://crrev.com/ce1f4d009316d8f5f4c49b18e1bd94687d80c59d/content/browser/android/web_contents_observer_proxy.h
,
Mar 14 2016
,
Mar 14 2016
Your change meets the bar and is auto-approved for M50 (branch: 2661)
,
Mar 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c278e82252a35dd31989c5e0b658b3d10788163d commit c278e82252a35dd31989c5e0b658b3d10788163d Author: Mikhail Naganov <mnaganov@chromium.org> Date: Mon Mar 14 23:45:44 2016 Store and use last base URL between DidStart / DidStopLoading As it turns out that the committed entry may omit the base URL if there has been a javascript: URL navigation after LoadDataWithBaseURL. Thus we have to store the base url on DidStartLoading and use it in DidStopLoading / DidFinishLoad. We have to preserve the old hack with SetToBaseURLForDataURLIfNeeded because if the base URL is not valid, DidFinishLoad doesn't receive it from the renderer. This change is a short-term workaround. The correct solution is to pass a flag that the load was through LoadDataWithBaseURL via Blink, so base URL can be restored correctly within NavigationController. BUG= 594001 Review URL: https://codereview.chromium.org/1779363004 Cr-Commit-Position: refs/heads/master@{#381108} (cherry picked from commit ce1f4d009316d8f5f4c49b18e1bd94687d80c59d) Review URL: https://codereview.chromium.org/1799973002 . Cr-Commit-Position: refs/branch-heads/2661@{#228} Cr-Branched-From: ef6f6ae5e4c96622286b563658d5cd62a6cf1197-refs/heads/master@{#378081} [modify] https://crrev.com/c278e82252a35dd31989c5e0b658b3d10788163d/android_webview/javatests/src/org/chromium/android_webview/test/LoadDataWithBaseUrlTest.java [modify] https://crrev.com/c278e82252a35dd31989c5e0b658b3d10788163d/content/browser/android/web_contents_observer_proxy.cc [modify] https://crrev.com/c278e82252a35dd31989c5e0b658b3d10788163d/content/browser/android/web_contents_observer_proxy.h
,
Mar 14 2016
Alex, do you think it makes sense to consider this change for M49 after testing on M50 first? I can imagine lots of apps may be calling `loadUrl("javascript:")` immediately after `loadDataWithBaseUrl`.
,
Mar 16 2016
This issue is impacting a large portion of MoPub customers as it's breaking specific ads rendering in the MoPub SDK and is causing a revenue loss for our app developers. Would be great to get this fixed in M49. Thank you!
,
Mar 16 2016
Bo has confirmed that his fix for another loadDataWithBaseURL problem (crbug.com/594611) doesn't involve any changes to the code changed in #6, so it can be merged into M49 independently.
,
Mar 16 2016
[Automated comment] Request affecting a post-stable build (M49), manual review required.
,
Mar 16 2016
Merge approved for M49 branch 2623.
,
Mar 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/78fca66434b0e5986b4501ea291175ae4b6b56df commit 78fca66434b0e5986b4501ea291175ae4b6b56df Author: Mikhail Naganov <mnaganov@chromium.org> Date: Wed Mar 16 20:14:27 2016 Store and use last base URL between DidStart / DidStopLoading As it turns out that the committed entry may omit the base URL if there has been a javascript: URL navigation after LoadDataWithBaseURL. Thus we have to store the base url on DidStartLoading and use it in DidStopLoading / DidFinishLoad. We have to preserve the old hack with SetToBaseURLForDataURLIfNeeded because if the base URL is not valid, DidFinishLoad doesn't receive it from the renderer. This change is a short-term workaround. The correct solution is to pass a flag that the load was through LoadDataWithBaseURL via Blink, so base URL can be restored correctly within NavigationController. BUG= 594001 Review URL: https://codereview.chromium.org/1779363004 Cr-Commit-Position: refs/heads/master@{#381108} (cherry picked from commit ce1f4d009316d8f5f4c49b18e1bd94687d80c59d) Review URL: https://codereview.chromium.org/1806853005 . Cr-Commit-Position: refs/branch-heads/2623@{#627} Cr-Branched-From: 92d77538a86529ca35f9220bd3cd512cbea1f086-refs/heads/master@{#369907} [modify] https://crrev.com/78fca66434b0e5986b4501ea291175ae4b6b56df/android_webview/javatests/src/org/chromium/android_webview/test/LoadDataWithBaseUrlTest.java [modify] https://crrev.com/78fca66434b0e5986b4501ea291175ae4b6b56df/content/browser/android/web_contents_observer_proxy.cc [modify] https://crrev.com/78fca66434b0e5986b4501ea291175ae4b6b56df/content/browser/android/web_contents_observer_proxy.h
,
Mar 16 2016
,
Mar 17 2016
Is there an ETA for when M49 will be redeployed with this fix, or has that already happened?
,
Mar 17 2016
> Is there an ETA for when M49 will be redeployed with this fix, or has that already happened? Not yet. Waiting for another crash fix. Hoping for next week.
,
Mar 22 2016
Hi there- pinging this thread to see if there are updates on ETA for push. Thank you.
,
Mar 22 2016
We pushed a respin of M49 yesterday, to the same users who received the previous M49 build.
,
Mar 30 2016
Thanks. Do you have the specific build of M49 after which this issue is fixed? We tested a few builds and still saw the issue. However, it was resolved in M50, so I think we were just testing incorrect versions of M49.
,
Mar 31 2016
49.0.2623.102 seems to be the first build with the merge from Comment 12.
,
Apr 4 2016
The fix should be available to all users at this point with 49.0.2623.105+ (which should be 100% released). Let us know if you can still repro. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by boliu@chromium.org
, Mar 11 2016