Bug 173613 - Website policies passed during redirects do not take effect
Summary: Website policies passed during redirects do not take effect
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-20 12:41 PDT by Matt Rajca
Modified: 2019-09-13 19:33 PDT (History)
5 users (show)

See Also:


Attachments
Patch (22.33 KB, patch)
2017-06-20 12:51 PDT, Matt Rajca
achristensen: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Rajca 2017-06-20 12:41:31 PDT
Website policies passed during redirects do not take effect and the original ones remain in effect. This happens because we cache these on the "policy document loader" which is null during a redirect. In that case, we fall back to the standard document loader. We ultimately end up caching our policies on two different document loaders, only one of which has effect. Since DocumentLoaders seem transient in nature, it might be better to cache these values on the Frame, as we do with the page zoom.
Comment 1 Matt Rajca 2017-06-20 12:51:28 PDT
Created attachment 313427 [details]
Patch
Comment 2 Darin Adler 2017-06-21 10:56:26 PDT
Comment on attachment 313427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313427&action=review

I am quite concerned that changing the semantics to make these policies immortal could be real problem. Is it really OK that these settings are going to persist across all kinds of navigations? Including the user going to entirely new webpages by following links or typing URLs?

> Source/WebCore/page/Frame.h:262
> +    bool userContentExtensionsEnabled() const { return m_userContentExtensionsEnabled; }
> +    void setUserContentExtensionsEnabled(bool enabled) { m_userContentExtensionsEnabled = enabled; }
> +
> +    AutoplayPolicy autoplayPolicy() const { return m_autoplayPolicy; }
> +    void setAutoplayPolicy(AutoplayPolicy policy) { m_autoplayPolicy = policy; }
> +
> +    OptionSet<AutoplayQuirk> allowedAutoplayQuirks() const { return m_allowedAutoplayQuirks; }
> +    void setAllowedAutoplayQuirks(OptionSet<AutoplayQuirk> allowedQuirks) { m_allowedAutoplayQuirks = allowedQuirks; }

Short version: These should all be on FrameLoader, not Frame.

Long version: This is not a good pattern for the future of WebKIt. We are trying to remove the many different responsibilities from Frame itself, and instead put these properties on appropriate frame sub-objects with more specific duties. I have been working for years to remove all the old things from Frame. Zoom factor is the perfect example of something I would like to move. Perhaps to FrameView, but not sure at this time exactly where I would move it. If moving from DocumentLoader, the most logical mechanical move to fix the lifetime would be to move from DocumentLoader to FrameLoader. And the code that sets these is already in FrameLoader so that makes even more sense.

> Source/WebCore/page/Frame.h:350
> +    bool m_userContentExtensionsEnabled { true };
> +    AutoplayPolicy m_autoplayPolicy { AutoplayPolicy::Default };
> +    OptionSet<AutoplayQuirk> m_allowedAutoplayQuirks;

Ditto.
Comment 3 Alex Christensen 2017-06-21 10:59:06 PDT
Comment on attachment 313427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313427&action=review

> Source/WebCore/loader/DocumentLoader.h:-482
> -    bool m_userContentExtensionsEnabled { true };

We put this on the DocumentLoader so that reloadWithoutContentBlockers would work correctly.  The MainFrame has a lifetime that is much longer than a DocumentLoader.
This change would need a test verifying that reloadWithoutContentBlockers followed by a reload and another reloadWithoutContentBlockers would work correctly.
Comment 4 Alex Christensen 2017-06-21 11:01:35 PDT
Comment on attachment 313427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313427&action=review

> Source/WebCore/ChangeLog:13
> +        loader". During a redirect, the policy document loader is NULL, in which case, we fall back to the
> +        standard document loader. We ultimately end up caching our policies on two different document loaders,
> +        only one of which has effect. Since DocumentLoaders seem transient in nature, it's better to cache
> +        these per-page settings on the Frame, as we currently do with the page zoom.

I think we should fix this problem some other way.  DocumentLoaders do come and go with different Documents, and that's what we want here.  Is there a way to the correct document loader at the correct time?
Comment 5 Matt Rajca 2017-06-21 14:32:03 PDT
I looked at this with Alex in-person and it would be better to fix this at the policy loader level. We also wouldn't have to change where we cache the website policies.