Bug 144269 – Add API to disable meta refreshes

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Brady Eidson]

Add a new NavigationType for client redirects

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

rdar://problem/20333198

[Brady Eidson]

Created attachment 251993 [details]
Patch v1

[Brady Eidson]

EWS for now. Will mark for review after that's all green.

[Alexey Proskuryakov]

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!

[Brady Eidson]

Created attachment 252062 [details]
Patch v2

[Brady Eidson]

We're going to go a different way with this: Add API to disable meta refreshes

[Brady Eidson]

Created attachment 252118 [details]
New approach - v1

[Alexey Proskuryakov]

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)?

[Brady Eidson]

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.

[Brady Eidson]

Oh yah, I definitely missed the WebCore::Settings changes. Yikes.

Will update with them tomorrow.

[Brady Eidson]

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.

[Alexey Proskuryakov]

> 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.

[Brady Eidson]

(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.

[Brady Eidson]

(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.

[Brady Eidson]

(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.  =/

[Brady Eidson]

Created attachment 252151 [details]
New approach v3

[Brady Eidson]

(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...

[Brady Eidson]

(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.

[Brady Eidson]

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.

[Alexey Proskuryakov]

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?

[Brady Eidson]

Created attachment 252156 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 252156 [details]
Patch for landing

Clearing flags on attachment: 252156

Committed r183682: <http://trac.webkit.org/changeset/183682>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.