Chrome app webview sending incorrect 'loadstart' events
Reported by
r...@rdharris.org,
Jan 13 2017
|
||||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36 Platform: 9000.50.0 (Official Build) beta-channel sumo Steps to reproduce the problem: 1. Create a chrome app with a webview where the source is a single page app (e.g. http://form.tab.com.au) 2. Add a load start event listener for the webview (e.g. webview.addEventListener('loadstart', function() {console.log('load started')) 3. Navigate to another URL which is handled by the app, i.e. doesn't require a new page to load What is the expected behavior? The loadstart event shouldn't be fired on the url change within the single page app, it should only fire when an new page is loaded. What went wrong? The loadstart event is fired Did this work before? Yes 55.0.2883.105 Does this work in other browsers? N/A Chrome version: 56.0.294.53 Channel: beta OS Version: 56.0.294.53 Flash Version: This is also happening for the 'loadstop' event. This only happens on the beta release 56.0.2924.58, it isn't happening on stable 55.0.2883.105. It is causing an issue for my app as it detects if webview content fails to load and displays a splash screen and reloads the page if that happens. One of the failure scenarios the app checks is if there is a 'loadstart' event for top level followed by a 'loadstop' event and no content is loaded in between the events. In 56.0.2924.58 every time a single page app changes url that scenario is triggered and the whole page is reloaded.
,
Jan 13 2017
#0, could you clarify what "Navigate to another URL which is handled by the app" means? I assume it means that the base URL does not change, i.e. host and URL path is not changed, just the param or '#' part changes. If so, it sounds like there is a behavior change from M55 to M56. 'loadstart' was not fired in M55 with such URL change but M56 the event is fired. mknowles@, could you help to triage or clarify what is the expected behavior of 'loadstart'/'loadstop'?
,
Jan 13 2017
,
Jan 14 2017
#2, I meant when the path changes using the html5 history API which is commonly used by single page app routing libraries like angular and react. https://staticapps.org/articles/routing-urls-in-static-apps/ So the path in the URL changes but the browsers does not request a to load a new page, the loaded page handle the change in URL to show new content. In M55 this change of URL didn't fire the 'loadstart' event but in M56 it does.
,
Jan 15 2017
,
Jan 16 2017
James - could you take a look to see what notifications may have changed for this type of navigation?
,
Jan 18 2017
Our team is severely affected by this. A team member has narrowed down the change in behavior to 56.0.2889.0 working correctly 56.0.2890.0 broken Is there agreement on this being a bug rather than a change of behavior keeping within spec? Seems to me that a loadstart event should not be fired unless the browser actually requests a load.
,
Jan 18 2017
Correct, it certainly looks like a bug. We're bisecting this now to see if we can find the root cause.
,
Jan 19 2017
I reverted https://chromium.googlesource.com/chromium/src/+/6adb8c23bd7b3fe24398c5500a2f89c758e36a4d locally and that fixed the issue.
,
Jan 19 2017
jam@ - Would you please take a look to see how this can be fixed?
,
Jan 19 2017
,
Jan 20 2017
If someone has a reduced repro they can attach that would be helpful.
,
Jan 20 2017
Here's what I used. Open the app and look at the console. If it just shows the loadstart for http://form.tab.com.au/ then it's good; if it shows an extra loadstart for http://form.tab.com.au/racing/home then it's bad.
,
Jan 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/840012cf9996d156f6e3286a37ca0c9d5d502edc commit 840012cf9996d156f6e3286a37ca0c9d5d502edc Author: jam <jam@chromium.org> Date: Fri Jan 20 20:38:26 2017 Fix regression of webview loadstart events firing for same page navigations. This regressed in r425073 since DidStartProvisionalLoadForFrame didn't fire for same page navigations while DidStartNavigation does. BUG= 680863 Review-Url: https://codereview.chromium.org/2647933002 Cr-Commit-Position: refs/heads/master@{#445150} [modify] https://crrev.com/840012cf9996d156f6e3286a37ca0c9d5d502edc/extensions/browser/guest_view/web_view/web_view_apitest.cc [modify] https://crrev.com/840012cf9996d156f6e3286a37ca0c9d5d502edc/extensions/browser/guest_view/web_view/web_view_guest.cc [add] https://crrev.com/840012cf9996d156f6e3286a37ca0c9d5d502edc/extensions/test/data/web_view/apitest/guest_same_page_navigation.html [modify] https://crrev.com/840012cf9996d156f6e3286a37ca0c9d5d502edc/extensions/test/data/web_view/apitest/main.js
,
Jan 20 2017
cederberg@ - Would your team like to test locally just to verify your issue is fixed by this CL? We should request merge once it's been through a canary release, perhaps the start of next week?
,
Jan 20 2017
Thanks! I'll build and verify that this fixes our issue.
,
Jan 20 2017
Sure, I will build and verify as soon as I see a release containing the CL. Thank you so much for acting on this so quickly. Much appreciated!
,
Jan 20 2017
aelder@ is more competent than me when it comes to this, so if he verifies then I am confident in the result.
,
Jan 20 2017
Spoke with wjmaclean@, this is not just Chrome OS but desktop as well. +bustamante for desktop.
,
Jan 20 2017
Confirmed. Synced past that commit and verified that fixes our issue. Thanks again!
,
Jan 20 2017
Thanks for verifying. I have a reminder to request merge on Tuesday morning.
,
Jan 23 2017
,
Jan 23 2017
(I won't do the merge till Tuesday, requesting now to be ready then)
,
Jan 23 2017
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions. Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 23 2017
Your change meets the bar and is auto-approved for M56. Please go ahead and merge the CL to branch 2924 manually. Please contact milestone owner if you have questions. Owners: amineer@(clank), cmasso@(bling), gkihumba@(cros), bustamante@(desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 23 2017
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions. Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9742bd9ee6833f78f23f7c3fb841b05e0c691612 commit 9742bd9ee6833f78f23f7c3fb841b05e0c691612 Author: John Abd-El-Malek <jam@chromium.org> Date: Mon Jan 23 18:34:04 2017 Fix regression of webview loadstart events firing for same page navigations. This regressed in r425073 since DidStartProvisionalLoadForFrame didn't fire for same page navigations while DidStartNavigation does. BUG= 680863 Review-Url: https://codereview.chromium.org/2647933002 Cr-Commit-Position: refs/heads/master@{#445150} (cherry picked from commit 840012cf9996d156f6e3286a37ca0c9d5d502edc) Review-Url: https://codereview.chromium.org/2646423003 . Cr-Commit-Position: refs/branch-heads/2924@{#843} Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059} [modify] https://crrev.com/9742bd9ee6833f78f23f7c3fb841b05e0c691612/extensions/browser/guest_view/web_view/web_view_apitest.cc [modify] https://crrev.com/9742bd9ee6833f78f23f7c3fb841b05e0c691612/extensions/browser/guest_view/web_view/web_view_guest.cc [add] https://crrev.com/9742bd9ee6833f78f23f7c3fb841b05e0c691612/extensions/test/data/web_view/apitest/guest_same_page_navigation.html [modify] https://crrev.com/9742bd9ee6833f78f23f7c3fb841b05e0c691612/extensions/test/data/web_view/apitest/main.js
,
Jan 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/852d488f12bf3778e57c4d7a50316c6006c4d108 commit 852d488f12bf3778e57c4d7a50316c6006c4d108 Author: John Abd-El-Malek <jam@chromium.org> Date: Mon Jan 23 18:43:28 2017 Fix regression of webview loadstart events firing for same page navigations. This regressed in r425073 since DidStartProvisionalLoadForFrame didn't fire for same page navigations while DidStartNavigation does. BUG= 680863 Review-Url: https://codereview.chromium.org/2647933002 Cr-Commit-Position: refs/heads/master@{#445150} (cherry picked from commit 840012cf9996d156f6e3286a37ca0c9d5d502edc) Review-Url: https://codereview.chromium.org/2649213002 . Cr-Commit-Position: refs/branch-heads/2987@{#29} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/852d488f12bf3778e57c4d7a50316c6006c4d108/extensions/browser/guest_view/web_view/web_view_apitest.cc [modify] https://crrev.com/852d488f12bf3778e57c4d7a50316c6006c4d108/extensions/browser/guest_view/web_view/web_view_guest.cc [add] https://crrev.com/852d488f12bf3778e57c4d7a50316c6006c4d108/extensions/test/data/web_view/apitest/guest_same_page_navigation.html [modify] https://crrev.com/852d488f12bf3778e57c4d7a50316c6006c4d108/extensions/test/data/web_view/apitest/main.js
,
Jan 23 2017
,
Jan 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/200122aeb0d45ce0b2943b82d7be22d4d16e1482 commit 200122aeb0d45ce0b2943b82d7be22d4d16e1482 Author: John Abd-El-Malek <jam@chromium.org> Date: Mon Jan 23 23:04:49 2017 Fix compile after recent merge The test harness changed on the branch since the fix, so the straight merge broke the build. BUG= 680863 Review-Url: https://codereview.chromium.org/2655523002 . Cr-Commit-Position: refs/branch-heads/2924@{#849} Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059} [modify] https://crrev.com/200122aeb0d45ce0b2943b82d7be22d4d16e1482/extensions/browser/guest_view/web_view/web_view_apitest.cc
,
Jan 25 2017
Tested the fix as per the steps in comment#13 on win10, mac 10.12.2, Linux 14.04 chrome version 56.0.2924.76 - Observed that console displays loadstart for http://form.tab.com.au/ as shown in the screenshot Fix works as expected |
||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||
Comment 1 by sdurais...@google.com
, Jan 13 2017