Bug 144269

Summary: Add API to disable meta refreshes
Product: WebKit Reporter: Brady Eidson <beidson>
Component: Page LoadingAssignee: 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 Flags
Patch v1
none
Patch v2
none
New approach - v1
none
New approach v2
none
New approach v3
none
Patch v4
ap: review+
Patch for landing none

Description Brady Eidson 2015-04-27 11:16:17 PDT
Add a new NavigationType for client redirects

This will help API clients distinguish these in the policy delegate.

rdar://problem/20333198
Comment 1 Brady Eidson 2015-04-29 15:24:40 PDT
Created attachment 251993 [details]
Patch v1
Comment 2 Brady Eidson 2015-04-29 15:24:59 PDT
EWS for now. Will mark for review after that's all green.
Comment 3 Alexey Proskuryakov 2015-04-29 16:45:01 PDT
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!
Comment 4 Brady Eidson 2015-04-30 09:19:12 PDT
Created attachment 252062 [details]
Patch v2
Comment 5 Brady Eidson 2015-04-30 15:18:25 PDT
We're going to go a different way with this: Add API to disable meta refreshes
Comment 6 Brady Eidson 2015-04-30 17:14:06 PDT
Created attachment 252118 [details]
New approach - v1
Comment 7 Alexey Proskuryakov 2015-04-30 17:20:27 PDT
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)?
Comment 8 Brady Eidson 2015-04-30 21:59:55 PDT
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.
Comment 9 Brady Eidson 2015-04-30 22:01:48 PDT
Oh yah, I definitely missed the WebCore::Settings changes. Yikes.

Will update with them tomorrow.
Comment 10 Brady Eidson 2015-04-30 22:50:52 PDT
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.
Comment 11 Alexey Proskuryakov 2015-04-30 23:01:18 PDT
> 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.
Comment 12 Brady Eidson 2015-05-01 08:07:06 PDT
(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.
Comment 13 Brady Eidson 2015-05-01 08:20:38 PDT
(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.
Comment 14 Brady Eidson 2015-05-01 08:29:15 PDT
(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.  =/
Comment 15 Brady Eidson 2015-05-01 08:49:18 PDT
Created attachment 252151 [details]
New approach v3
Comment 16 Brady Eidson 2015-05-01 08:50:20 PDT
(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...
Comment 17 Brady Eidson 2015-05-01 09:24:45 PDT
(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.
Comment 18 Brady Eidson 2015-05-01 09:35:20 PDT
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 19 Alexey Proskuryakov 2015-05-01 10:21:07 PDT
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?
Comment 20 Brady Eidson 2015-05-01 10:33:35 PDT
Created attachment 252156 [details]
Patch for landing
Comment 21 WebKit Commit Bot 2015-05-01 11:21:16 PDT
Comment on attachment 252156 [details]
Patch for landing

Clearing flags on attachment: 252156

Committed r183682: <http://trac.webkit.org/changeset/183682>
Comment 22 WebKit Commit Bot 2015-05-01 11:21:22 PDT
All reviewed patches have been landed.  Closing bug.