Bug 207427

Created attachment 390173 [details] Patch

Created attachment 390178 [details] Patch

Created attachment 390181 [details] Patch

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.

Created attachment 390191 [details] Patch

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

Created attachment 390195 [details] Patch

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

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

Well it's actually SPI on WKWebViewConfiguration, so that's potentially much more problematic.

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.

Created attachment 390346 [details] Patch

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

Created attachment 390669 [details] Patch

Created attachment 390673 [details] Patch

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.

Created attachment 390731 [details] Patch Trying this approach on the bots (fixed at least some tests locally)

Created attachment 390768 [details] Patch Trying this approach on the bots (fixed more tests locally)

Created attachment 390784 [details] Patch

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?

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

Created attachment 390855 [details] Patch

(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 on attachment 390855 [details] Patch Clearing flags on attachment: 390855 Committed r256715: <https://trac.webkit.org/changeset/256715>

All reviewed patches have been landed. Closing bug.

<rdar://problem/59498009>

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)