Summary: | Web Inspector: breakpoint actions should work regardless of Content Security Policy | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||||||
Component: | Web Inspector | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, graouts, joepeck, mark.lam, timothy, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Matt Baker
2014-09-04 12:48:43 PDT
Created attachment 237664 [details]
Patch
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. Created attachment 237670 [details]
Patch
Comment on attachment 237670 [details]
Patch
r=me
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 on attachment 237670 [details]
Patch
Cancelling commit so that Matt can address Joe's comments.
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. Created attachment 237702 [details]
Patch
Created attachment 237704 [details]
Patch
Comment on attachment 237704 [details]
Patch
r=me
Comment on attachment 237704 [details] Patch Clearing flags on attachment: 237704 Committed r173333: <http://trac.webkit.org/changeset/173333> All reviewed patches have been landed. Closing bug. |