Bug 207427

Summary: Expose "allowsContentJavaScript" on WKWebpagePreferences
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit APIAssignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez, commit-queue, darin, dbates, esprehn+autocc, ews-watchlist, japhet, kangil.han, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Brady Eidson 2020-02-08 10:26:12 PST
Expose "allowsContentJavaScript" on WKWebpagePreferences

This will allow content (ne "markup") JS to be disabled while still allowing script from API entry points.

<rdar://problem/51534967>
Comment 1 Brady Eidson 2020-02-08 10:31:13 PST
Created attachment 390173 [details]
Patch
Comment 2 Brady Eidson 2020-02-08 13:09:19 PST
Created attachment 390178 [details]
Patch
Comment 3 Brady Eidson 2020-02-08 13:53:57 PST
Created attachment 390181 [details]
Patch
Comment 4 Sam Weinig 2020-02-08 15:19:14 PST
Comment on attachment 390181 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.h:650
> +    Optional<bool> m_allowsScriptMarkup;

I find Optional<bool> to be hard to reason about and not all that self documenting. In other cases I have been recommending a three state enum instead.
Comment 5 Brady Eidson 2020-02-08 17:56:21 PST
Created attachment 390191 [details]
Patch
Comment 6 Brady Eidson 2020-02-08 17:57:31 PST
(In reply to Sam Weinig from comment #4)
> Comment on attachment 390181 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390181&action=review
> 
> > Source/WebCore/loader/DocumentLoader.h:650
> > +    Optional<bool> m_allowsScriptMarkup;
> 
> I find Optional<bool> to be hard to reason about and not all that self
> documenting. In other cases I have been recommending a three state enum
> instead.

Fair enough.
Comment 7 Brady Eidson 2020-02-08 21:36:31 PST
Created attachment 390195 [details]
Patch
Comment 8 Darin Adler 2020-02-09 21:17:55 PST
Comment on attachment 390195 [details]
Patch

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

Looks generally good. I have some questions so I didn’t review+ or review-.

> Source/WebCore/loader/DocumentLoader.cpp:1227
> +    if (m_scriptMarkupPolicy != ScriptMarkupPolicy::Default)
> +        m_frame->settings().setScriptMarkupEnabled(m_scriptMarkupPolicy == ScriptMarkupPolicy::Enable);

I’m concerned that applyPoliciesToSettings is not a maintainable approach long term. Things that are different-per-document-loader probably should not be stored in settings that hang off the frame.

Separate thought: The behavior of ScriptMarkupPolicy::Default seems to be "leave the settings behind from the previous document"? Is that safe?

> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.mm:347
> +    switch (_websitePolicies->scriptMarkupPolicy()) {
> +    case WebCore::ScriptMarkupPolicy::Default:
> +    case WebCore::ScriptMarkupPolicy::Enable:
> +        return true;
> +    case WebCore::ScriptMarkupPolicy::Disable:
> +        return false;
> +    }

If you never set this property, the markup policy is different from either NO or YES. But we do not document this difference. Nor offer a way to restore this behavior. I guess this third policy means "respect a preference with no API but that might be set on the command line"?
Comment 9 Brady Eidson 2020-02-09 22:01:14 PST
(In reply to Darin Adler from comment #8)
> Comment on attachment 390195 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390195&action=review
> 
> Looks generally good. I have some questions so I didn’t review+ or review-.
> 
> > Source/WebCore/loader/DocumentLoader.cpp:1227
> > +    if (m_scriptMarkupPolicy != ScriptMarkupPolicy::Default)
> > +        m_frame->settings().setScriptMarkupEnabled(m_scriptMarkupPolicy == ScriptMarkupPolicy::Enable);
> 
> I’m concerned that applyPoliciesToSettings is not a maintainable approach
> long term. Things that are different-per-document-loader probably should not
> be stored in settings that hang off the frame.

In this case, "any document loader in the page getting this" should apply to the page global settings.

I 100% agree that this approach doesn't seem as general purpose as as some of the other users of it!

> Separate thought: The behavior of ScriptMarkupPolicy::Default seems to be
> "leave the settings behind from the previous document"? Is that safe?

The *intention* was "If WKPreferencesRef or WKBrowsingContextGroup users had set the markup-enabled flag, and nobody set it on the WKWebsitePreferences, respect the SPI setting"

But in those cases, you are 100% correct that this is what happens in effect.

The SPI override really gets in the way here.

I'll either determine the SPI support is no longer needed and greatly simplify this code, or I'll adjust to handle it.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.mm:347
> > +    switch (_websitePolicies->scriptMarkupPolicy()) {
> > +    case WebCore::ScriptMarkupPolicy::Default:
> > +    case WebCore::ScriptMarkupPolicy::Enable:
> > +        return true;
> > +    case WebCore::ScriptMarkupPolicy::Disable:
> > +        return false;
> > +    }
> 
> If you never set this property, the markup policy is different from either
> NO or YES. But we do not document this difference. Nor offer a way to
> restore this behavior. I guess this third policy means "respect a preference
> with no API but that might be set on the command line"?

Indeed - this trinary state came about because of a conflict with the SPI code paths to this setting.

As mentioned above, I'll figure out the way forward.
Comment 10 Brady Eidson 2020-02-09 22:06:13 PST
Well it's actually SPI on WKWebViewConfiguration, so that's potentially much more problematic.
Comment 11 Brady Eidson 2020-02-09 22:11:42 PST
Yup, definitely still have to support that SPI.

The "dangerous" thing is accidentally allowing markup JS when somebody has asked the view to not allow it.

I can do that with Settings + FrameLoaderClient instead of this ternary state.
Comment 12 Brady Eidson 2020-02-10 22:42:34 PST
Created attachment 390346 [details]
Patch
Comment 13 Brady Eidson 2020-02-10 22:48:10 PST
(In reply to Brady Eidson from comment #9)
> (In reply to Darin Adler from comment #8
> 
> In this case, "any document loader in the page getting this" should apply to
> the page global settings.

This got me thinking as to my approach, which is now to put the "did this navigation have the markup disabled policy?" flag on the Page itself, living side-by-side with the WKPreferences-driven Settings version.

Then we consult both at decision time.
Comment 14 Brady Eidson 2020-02-13 10:49:47 PST
Created attachment 390669 [details]
Patch
Comment 15 Brady Eidson 2020-02-13 11:46:45 PST
Created attachment 390673 [details]
Patch
Comment 16 Brady Eidson 2020-02-13 22:07:58 PST
Ouch. The key with these layout test failures is:

bool Document::scriptMarkupEnabled() const
{
    if (!settings().scriptMarkupEnabled())
        return false;

    if (!m_frame || m_frame->document() != this)
        return false;
    return m_frame->loader().client().scriptMarkupPolicyFromMostRecentNavigation() == ScriptMarkupPolicy::Enable;
}

The middle clause of no frame.

I'd figured "A frameless document should never be executing script, right?"

But I was totally wrong.

So now I have to find a (good) way to propagate the policy forward to frameless documents from whoever creates them.
Comment 17 Brady Eidson 2020-02-13 22:50:59 PST
Created attachment 390731 [details]
Patch

Trying this approach on the bots (fixed at least some tests locally)
Comment 18 Brady Eidson 2020-02-14 07:53:33 PST
Created attachment 390768 [details]
Patch

Trying this approach on the bots (fixed more tests locally)
Comment 19 Brady Eidson 2020-02-14 10:23:15 PST
Created attachment 390784 [details]
Patch
Comment 20 Darin Adler 2020-02-14 15:16:04 PST
Comment on attachment 390784 [details]
Patch

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

I like the public allowsContentJavaScript name so much better than the internal scriptMarkupPolicy name.

> Source/WebCore/dom/Document.cpp:6817
> +    if (!settings().scriptMarkupEnabled())

Seems slightly dangerous to still have this Settings::scriptMarkupEnabled function. If anyone calls it and it returns true, they will likely do the wrong thing. Maybe at some point we can rename it?

> Source/WebCore/dom/Document.cpp:6826
> +        if (m_contextDocument)
> +            return m_contextDocument->scriptMarkupEnabled();
> +
> +        // If this Document is frameless or in the wrong frame, *and* it has no context document,
> +        // then it has no business running markup script.
> +        return false;

I’d find this slightly easier to read as an && expression:

    return m_contextDocument && m_contextDocument->scriptMarkupEnabled();

Also slightly annoying that there’s recursion here, but I suppose it’s unlikely to recurse deeply.

> Source/WebCore/xml/XMLHttpRequest.cpp:194
>              m_responseDocument->setContextDocument(context);
> +            m_responseDocument->setContent(m_responseBuilder.toStringPreserveCapacity());
>              m_responseDocument->setSecurityOriginPolicy(context.securityOriginPolicy());
>              m_responseDocument->overrideMIMEType(mimeType);

Maybe we should also move up the setSecurityOriginPolicy and overrideMIMEType calls up before we call setContent too? Seems like we’d want all those things set before we do any parsing.

> Source/WebKit/WebProcess/WebPage/WebFrame.cpp:269
> +        ASSERT(page());

What guarantees page() is not null?
Comment 21 Brady Eidson 2020-02-14 18:27:44 PST
(In reply to Darin Adler from comment #20)
> Comment on attachment 390784 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390784&action=review
> 
> I like the public allowsContentJavaScript name so much better than the
> internal scriptMarkupPolicy name.

Me too. It was originally the tri-state enum Sam requested and this "FooPolicy" name was matching other tri-state style.

I'll rename.

> 
> > Source/WebCore/dom/Document.cpp:6817
> > +    if (!settings().scriptMarkupEnabled())
> 
> Seems slightly dangerous to still have this Settings::scriptMarkupEnabled
> function. If anyone calls it and it returns true, they will likely do the
> wrong thing. Maybe at some point we can rename it?

I agree! Unfortunately it is exposed as (multiple) SPIs that are still used.

Renaming it to something that discourages its use seems great.

> > Source/WebCore/dom/Document.cpp:6826
> > +        if (m_contextDocument)
> > +            return m_contextDocument->scriptMarkupEnabled();
> > +
> > +        // If this Document is frameless or in the wrong frame, *and* it has no context document,
> > +        // then it has no business running markup script.
> > +        return false;
> 
> I’d find this slightly easier to read as an && expression:
> 
>     return m_contextDocument && m_contextDocument->scriptMarkupEnabled();
> 
> Also slightly annoying that there’s recursion here, but I suppose it’s
> unlikely to recurse deeply.
> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:194
> >              m_responseDocument->setContextDocument(context);
> > +            m_responseDocument->setContent(m_responseBuilder.toStringPreserveCapacity());
> >              m_responseDocument->setSecurityOriginPolicy(context.securityOriginPolicy());
> >              m_responseDocument->overrideMIMEType(mimeType);
> 
> Maybe we should also move up the setSecurityOriginPolicy and
> overrideMIMEType calls up before we call setContent too? Seems like we’d
> want all those things set before we do any parsing.

Sounds right!

> 
> > Source/WebKit/WebProcess/WebPage/WebFrame.cpp:269
> > +        ASSERT(page());
> 
> What guarantees page() is not null?

The coreFrame null check earlier almost always guarantees this.
It's not a 100% guarantee, I'll clean it up.
Comment 22 Brady Eidson 2020-02-14 22:37:48 PST
Created attachment 390855 [details]
Patch
Comment 23 Brady Eidson 2020-02-15 08:08:37 PST
(In reply to Brady Eidson from comment #22)
> Created attachment 390855 [details]
> Patch

This will be for landing later: Some clients need to handle the deprecation first
Comment 24 WebKit Commit Bot 2020-02-16 17:35:23 PST
Comment on attachment 390855 [details]
Patch

Clearing flags on attachment: 390855

Committed r256715: <https://trac.webkit.org/changeset/256715>
Comment 25 WebKit Commit Bot 2020-02-16 17:35:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 Radar WebKit Bug Importer 2020-02-16 17:36:19 PST
<rdar://problem/59498009>
Comment 27 Alex Christensen 2020-02-17 11:39:17 PST
Comment on attachment 390855 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEvaluateJavaScript.mm:261
> +TEST(WebKit, AllowsContentJavaScript)

This needs #if HAVE(NETWORK_FRAMEWORK)