Summary: | Add API to disable meta refreshes | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||||||||||
Component: | Page Loading | Assignee: | Brady Eidson <beidson> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | ap, commit-queue, esprehn+autocc, japhet, kangil.han, sam | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Bug Depends on: | 144270, 144276 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Brady Eidson
2015-04-27 11:16:17 PDT
Created attachment 251993 [details]
Patch v1
EWS for now. Will mark for review after that's all green. Comment on attachment 251993 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=251993&action=review > LayoutTests/fast/loader/redirect-to-invalid-url-using-meta-refresh-calls-policy-delegate-expected.txt:1 > +Policy delegate: attempt to load http://A=a&B=b with navigation type 'client redirect' I don't understand whether this is supposed to include setting window.location or not. Can we just call this "meta refresh" instead? > Source/WebCore/dom/Document.cpp:3014 > + frame->navigationScheduler().scheduleRedirect(delay, completedURL, NavigationScheduler::RedirectType::Client); Maybe also "NavigationScheduler::RedirectType::MetaRefresh"? > Source/WebCore/loader/FrameLoadRequest.h:49 > + , m_frameLoadTypeIsSet(false) I think that we use Optional for that now. > Source/WebCore/loader/FrameLoaderTypes.h:64 > + ClientRedirect, > + ClientRedirectWithLockedBackForwardList, This one is really suspicious to me. It doesn't seem orthogonal to the other values! Created attachment 252062 [details]
Patch v2
We're going to go a different way with this: Add API to disable meta refreshes Created attachment 252118 [details]
New approach - v1
Comment on attachment 252118 [details] New approach - v1 View in context: https://bugs.webkit.org/attachment.cgi?id=252118&action=review > LayoutTests/loader/meta-refresh-disabled.html:4 > + testRunner.overridePreference("WebKitMetaRefreshEnabled", "0"); I think that you need to update WKTR/DRT for this to not break all the tests. Also, this probably doesn't work on Windows. > LayoutTests/loader/meta-refresh-disabled.html:15 > + if (window.testRunner) > + setTimeout("testRunner.notifyDone();", 100); > +} > +</script> > +<meta http-equiv="refresh" content="0;url=resources/notify-done.html"> This feels somewhat flaky... Maybe the meta could go before the script (and then you'd need dumpAsText in the target too)? Quite confused about the platform-wide build failures as I build and ran tests fine locally. Maybe I missed a `git add` or something. (In reply to comment #7) > Comment on attachment 252118 [details] > New approach - v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252118&action=review > > > LayoutTests/loader/meta-refresh-disabled.html:4 > > + testRunner.overridePreference("WebKitMetaRefreshEnabled", "0"); > > I think that you need to update WKTR/DRT for this to not break all the > tests. As mentioned on IRC, tests ran fine locally. I was under the impression DRT/WKTR had some logic to reset all prefs, based on an initial pref snapshot. I might be wrong and got lucky somehow. > Also, this probably doesn't work on Windows. Yup, Windows definitely needs to skip the new test. > > LayoutTests/loader/meta-refresh-disabled.html:15 > > + if (window.testRunner) > > + setTimeout("testRunner.notifyDone();", 100); > > +} > > +</script> > > +<meta http-equiv="refresh" content="0;url=resources/notify-done.html"> > > This feels somewhat flaky... Maybe the meta could go before the script (and > then you'd need dumpAsText in the target too)? We do already have loader tests - even specifically <meta refresh> ones - that take awhile. But I know you're indicating that the arbitrary 100ms delay is flakey. Unfortunately I couldn't come up with an obviously better way to do this... I'm not sure why you think putting the <meta> before the <script> would help. Maybe your understanding is that the <meta refresh> would be synchronous and therefore that would lead to deterministic behavior? Because that's not the case - this <meta> is on a 0-delay timer that only gets set once the page load is done. Oh yah, I definitely missed the WebCore::Settings changes. Yikes. Will update with them tomorrow. Created attachment 252134 [details]
New approach v2
Not for review yet - Just want to wake up tomorrow and have EWS tell me the build worked.
> As mentioned on IRC, tests ran fine locally. I was under the impression > DRT/WKTR had some logic to reset all prefs, based on an initial pref > snapshot. Let's find where that's done. > Maybe your understanding is that the <meta refresh> would be synchronous and > therefore that would lead to deterministic behavior? Because that's not the > case - this <meta> is on a 0-delay timer that only gets set once the page > load is done. I didn't know that the 0-delay timer was set once the page load was done, not when <meta> was parsed. This is strange - is that what the spec requires, or a random WebKit bug? Why would the redirect want to wait until the page is loaded? If the 0-delay timer started immediately, the test would be obviously racy. And if it happens after onload, we shouldn't need a 100 ms delay. The rule of thumb is that all timers in tests should be 0-delay, anything else needs proof of correctness. (In reply to comment #11) > > As mentioned on IRC, tests ran fine locally. I was under the impression > > DRT/WKTR had some logic to reset all prefs, based on an initial pref > > snapshot. > > Let's find where that's done. > > > Maybe your understanding is that the <meta refresh> would be synchronous and > > therefore that would lead to deterministic behavior? Because that's not the > > case - this <meta> is on a 0-delay timer that only gets set once the page > > load is done. > > I didn't know that the 0-delay timer was set once the page load was done, > not when <meta> was parsed. This is strange - is that what the spec > requires, or a random WebKit bug? Why would the redirect want to wait until > the page is loaded? > > If the 0-delay timer started immediately, the test would be obviously racy. I was wrong about it being when the page load is done. But it is set immediately, which - as you say - makes things racy. (In reply to comment #11) > > As mentioned on IRC, tests ran fine locally. I was under the impression > > DRT/WKTR had some logic to reset all prefs, based on an initial pref > > snapshot. > > Let's find where that's done. This works automatically in WK2 because WKTR calls WKPreferencesResetTestRunnerOverrides between tests. This forces a fresh copy of prefs from the UI process down to the WebProcess before the next test starts. In WK1, DRT needs to know each individual pref to reset. Doing that now. (In reply to comment #7) > Comment on attachment 252118 [details] > New approach - v1 > > + if (window.testRunner) > > + setTimeout("testRunner.notifyDone();", 100); > > +} > > +</script> > > +<meta http-equiv="refresh" content="0;url=resources/notify-done.html"> > > This feels somewhat flaky... Maybe the meta could go before the script (and > then you'd need dumpAsText in the target too)? Also, another comment on this: The meta is *already* before the script that sets the timeout to do notify done, as that script fires on load, which is well after the meta has processed. =/ Created attachment 252151 [details]
New approach v3
(In reply to comment #15) > Created attachment 252151 [details] > New approach v3 Re: the test... I'm really not sure what a better way to do this is, but am open to ideas... (In reply to comment #12) > (In reply to comment #11) > > > As mentioned on IRC, tests ran fine locally. I was under the impression > > > DRT/WKTR had some logic to reset all prefs, based on an initial pref > > > snapshot. > > > > Let's find where that's done. > > > > > Maybe your understanding is that the <meta refresh> would be synchronous and > > > therefore that would lead to deterministic behavior? Because that's not the > > > case - this <meta> is on a 0-delay timer that only gets set once the page > > > load is done. > > > > I didn't know that the 0-delay timer was set once the page load was done, > > not when <meta> was parsed. This is strange - is that what the spec > > requires, or a random WebKit bug? Why would the redirect want to wait until > > the page is loaded? > > > > If the 0-delay timer started immediately, the test would be obviously racy. > > I was wrong about it being when the page load is done. But it is set > immediately, which - as you say - makes things racy. Debugging further... Actually, no - The redirect is scheduled right away, in that the "ScheduleRedirect" is pushed on the nav scheduler's list of actions... but the actual 0-delay timer to trigger it in the future is *NOT* started right away. It is started when the page finishes loading, but *after* the onload event fires. So the 0-delay for the "notifyDone" takes place before the 0-delay for the redirect itself. So to make this test be less hacky than using an arbitrary 100ms delay, I could just double-delay the setTimeout (0 delay, then another 0 delay). But that approach itself is hacky, because it relies on this understanding of how WebKit acts today. Created attachment 252153 [details]
Patch v4
This approach is slightly better that waiting an arbitrary 100ms, but it is still somewhat arbitrary and still somewhat hacky.
Comment on attachment 252153 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=252153&action=review > LayoutTests/loader/resources/notify-done.html:3 > +if (window.testRunner) > + testRunner.notifyDone(); Maybe add FAIL text here? Created attachment 252156 [details]
Patch for landing
Comment on attachment 252156 [details] Patch for landing Clearing flags on attachment: 252156 Committed r183682: <http://trac.webkit.org/changeset/183682> All reviewed patches have been landed. Closing bug. |