Bug 136542 - Web Inspector: breakpoint actions should work regardless of Content Security Policy
Summary: Web Inspector: breakpoint actions should work regardless of Content Security ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-09-04 12:48 PDT by Matt Baker
Modified: 2014-09-05 14:08 PDT (History)
6 users (show)

See Also:


Attachments
Patch (15.59 KB, patch)
2014-09-04 17:59 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (16.02 KB, patch)
2014-09-04 19:17 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (15.86 KB, patch)
2014-09-05 11:42 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (15.97 KB, patch)
2014-09-05 12:17 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2014-09-04 12:48:43 PDT
Breakpoint actions that evaluate JavaScript don't work when inspecting pages with a CSP that doesn't permit "unsafe-eval".
Comment 1 Radar WebKit Bug Importer 2014-09-04 12:49:00 PDT
<rdar://problem/18233842>
Comment 2 Matt Baker 2014-09-04 17:59:34 PDT
Created attachment 237664 [details]
Patch
Comment 3 Mark Lam 2014-09-04 18:45:06 PDT
Comment on attachment 237664 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        JSGlobalObject for the duration of scope, returning the eval enabled state to its

/duration of scope/duration of a scope/

> Source/JavaScriptCore/ChangeLog:11
> +        to allow breakpoint actions to execute JS in pages with CSP that would normally

Would you mind spelling out Content Security Policy instead of the CSP abbreviation?  Thanks.

> Source/JavaScriptCore/ChangeLog:14
> +        Refactored Inspector::InjectedScriptBase to use RAII object instead of manually

/use RAII/use the RAII/

> Source/JavaScriptCore/ChangeLog:15
> +        setting/resetting eval enabled state.

/...ing eval enabled/...ing the eval enabled/

> Source/JavaScriptCore/debugger/DebuggerEvalEnabler.h:46
> +    explicit DebuggerEvalEnabler(const ExecState* exec)
> +        : m_globalObject(nullptr)
> +        , m_evalEnabled(false)
> +    {
> +        if (exec) {
> +            m_globalObject = exec->lexicalGlobalObject();
> +            m_evalEnabled = m_globalObject->evalEnabled();
> +        }
> +        if (m_globalObject && !m_evalEnabled)
> +            m_globalObject->setEvalEnabled(true, m_globalObject->evalDisabledErrorMessage());
> +    }

I suggest renaming m_evalEnabled to m_evalWasEnabled to indicate that it is referring to the original state.  Actually, I think you should use m_evalwasDisabled instead because that would remove the need for a lot of negation in the code (making the logic simpler).

I think it's weird that the exec can be NULL, but that appears to be a due to InjectedScriptBase getting the exec from ScriptObject::scriptState() which can currently be NULL.  I suggest adding a comment in the ChangeLog to document this so that we'll have a record of why this NULL check exists, and can remove it in the future if we fix ScriptObject::scriptState() to never be NULL (at least for the duration when the DebuggerEvalEnabler is called).

Why not just cache the ExecState, and do a conditional on that?  I suggest the following:

    explicit DebuggerEvalEnabler(const ExecState* exec)
        : m_exec(exec)
        , m_evalWasDisabled(false)
    {
        if (exec) {
            JSGlobalObject* globalObject = exec->lexicalGlobalObject();
            m_evalWasDisabled = !m_globalObject->evalEnabled();
            if (m_evalWasDisabled)
                globalObject->setEvalEnabled(true, globalObject->evalDisabledErrorMessage());
        }
    }

    ~DebuggerEvalEnabler()
    {
        if (m_evalWasDisabled) {
            ASSERT(m_exec);
            JSGlobalObject* globalObject = exec->lexicalGlobalObject();
            globalObject()->setEvalEnabled(false, globalObject->evalDisabledErrorMessage());
        }
    }

I think the code is a little simpler this way.
Comment 4 Matt Baker 2014-09-04 19:17:21 PDT
Created attachment 237670 [details]
Patch
Comment 5 Mark Lam 2014-09-04 19:33:28 PDT
Comment on attachment 237670 [details]
Patch

r=me
Comment 6 Joseph Pecoraro 2014-09-04 19:36:23 PDT
Comment on attachment 237670 [details]
Patch

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

Looks good to me.

> Source/JavaScriptCore/debugger/DebuggerEvalEnabler.h:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

Nit: 2014

> LayoutTests/inspector/debugger/breakpoint-action-eval.html:22
> +    WebInspector.Frame.addEventListener(WebInspector.Frame.Event.MainResourceDidChange, function() {

Is the MainResourceDidChange + reload really necessary? This test would be much simpler if it was just listening for the ScriptAdded event.

> LayoutTests/inspector/debugger/breakpoint-action-eval.html:39
> +          });

Nit: broken indentation.
Comment 7 Mark Lam 2014-09-04 19:37:32 PDT
Comment on attachment 237670 [details]
Patch

Cancelling commit so that Matt can address Joe's comments.
Comment 8 Mark Lam 2014-09-05 11:19:45 PDT
Comment on attachment 237670 [details]
Patch

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

> Source/JavaScriptCore/inspector/InjectedScriptBase.cpp:86
> +    JSC::DebuggerEvalEnabler evalEnabler(scriptState);
>      Deprecated::ScriptValue resultValue = function.call(hadException);

Please add { } around these lines so that we preserve the original semantics of only enabling evals for this function call, and not the didCallInjectedScriptFunction() callback that follows below.
Comment 9 Matt Baker 2014-09-05 11:42:55 PDT
Created attachment 237702 [details]
Patch
Comment 10 Matt Baker 2014-09-05 12:17:02 PDT
Created attachment 237704 [details]
Patch
Comment 11 Mark Lam 2014-09-05 13:31:36 PDT
Comment on attachment 237704 [details]
Patch

r=me
Comment 12 WebKit Commit Bot 2014-09-05 14:08:26 PDT
Comment on attachment 237704 [details]
Patch

Clearing flags on attachment: 237704

Committed r173333: <http://trac.webkit.org/changeset/173333>
Comment 13 WebKit Commit Bot 2014-09-05 14:08:29 PDT
All reviewed patches have been landed.  Closing bug.