Bug 32063 – ScriptController::isEnabled needs to be renamed

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Patrik Persson]

During the work with #21288, we realized that the method ScriptController::isEnabled() has grown to do more than the name says. The name should reflect what the method does (checks settings, checks sandboxing status, passes decision to frame loader client): 

bool ScriptController::isEnabled()
{
    Settings* settings = m_frame->settings();
    return m_frame->loader()->client()->allowJavaScript(settings && settings->isJavaScriptEnabled() && !m_frame->loader()->isSandboxed(SandboxScripts));
}

Darin Adler suggested 'canExecuteScripts' below. Is that OK?

(In reply to comment #51 in bug 21288)
> Loose end for after this lands: ScriptController::isEnabled needs to be
> renamed. It already checked more than just whether JavaScript was enabled.
> Maybe canExecuteScripts is the right name.

[Alexey Proskuryakov]

> Darin Adler suggested 'canExecuteScripts' below. Is that OK?

The name looks good in abstract. Once you post  a final patch, it will be easier to see if it makes sense at all call sites.

    return m_frame->loader()->client()->allowJavaScript(settings &&
settings->isJavaScriptEnabled() &&
!m_frame->loader()->isSandboxed(SandboxScripts));

By the way, given that the argument to FrameLoaderClient::allowJavaScript() is called "enabledPerSettings", it may be more appropriate to have the new isSandboxed() check outside the parentheses.

[Darin Adler]

(In reply to comment #1)
> By the way, given that the argument to FrameLoaderClient::allowJavaScript() is
> called "enabledPerSettings", it may be more appropriate to have the new
> isSandboxed() check outside the parentheses.

Patrik, sorry for sidetracking in your bug.

Alexey, I think the real issue there is whether we want the frame loader client to be able to override the sandboxing.

If we do want the client to have this capability, and I personally think we do, then we could change the name of the argument to allowJavaScript. Or consider other API refinements so the client can make the decision in a well-informed way.

If we don't, then I think we should change the rules so that ScriptController won't even call allowJavaScript when sandboxed.

We could look at what clients currently do with the allowJavaScript function to inform that decision.

[Adam Barth]

Why would you want the client to be able to override the sandbox?  I'm not opposed, I just don't understand the use case.

[Darin Adler]

(In reply to comment #3)
> Why would you want the client to be able to override the sandbox?

I'm not sure there is a use case.

I think it depends on why we have allowJavaScript as a client call at all. This was added for Chromium and is currently not used in any other port.

[Adam Barth]

fishd would know, but I think this was due to paranoia about XSS in browser UI surfaces built out of HTML.

[Adam Barth]

Created attachment 46202 [details]
Patch

[WebKit Review Bot]

Attachment 46202 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/176231

[Adam Barth]

Created attachment 46227 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 46227 [details]
Patch

Rejecting patch 46227 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11902 test cases.
fast/profiler/profile-calls-in-included-file.html -> crashed

Exiting early after 1 failures. 7848 tests run.
270.16s total testing time

7847 test cases (99%) succeeded
1 test case (<1%) crashed
3 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/175716

[Adam Barth]

Comment on attachment 46227 [details]
Patch

Clearing flags on attachment: 46227

Committed r53046: <http://trac.webkit.org/changeset/53046>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Filed bug 33490 about the crash.

[Darin Fisher (:fishd, Google)]

(In reply to comment #5)
> > I think it depends on why we have allowJavaScript as a client call at all. 
> > This was added for Chromium and is currently not used in any other port.
> fishd would know, but I think this was due to paranoia about XSS in browser UI
> surfaces built out of HTML.

Chromium wants to be able to enable JavaScript for chrome:// pages as well as file:// and ftp:// directory listings even when the user has disabled JavaScript.

I believe allowJavaScript is also enables a feature like the noscript Firefox extension.