DevTools: respond with error when Debugger command is sent to disabled debugger agent

DevTools: respond with error when Debugger command is sent to disabled debugger agent

InspectorBackendDispatcher now will respond with error message if debugger command arrives to disabled debugger agent.

DOMDebugger will also respond with an error on attempt to add breakpoint if DOM or Debugger is not enabled.

DebuggerScript.js is only compiled when ScriptDebugServer::enabled is called. No lazy compilation anymore.

BUG=481845
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195053

[yurys, 2015-05-05 12:32:05]

yurys@chromium.org changed reviewers:
+ dgozman@chromium.org, kozyatinskiy@chromium.org, pfeldman@chromium.org

[yurys, 2015-05-05 12:32:19]

yurys@chromium.org changed reviewers:
+ sergeyv@chromium.org

[sergeyv, 2015-05-05 12:41:10]

lets add the similar check to domdebuggerAgent

https://codereview.chromium.org/1129473003/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptDebugServer.h (right):

https://codereview.chromium.org/1129473003/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptDebugServer.h:63: static bool
isDebugContext(v8::Local<v8::Context>);
?

[yurys, 2015-05-05 13:13:02]

Added similar check for DOMDebugger.

https://codereview.chromium.org/1129473003/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptDebugServer.h (right):

https://codereview.chromium.org/1129473003/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptDebugServer.h:63: static bool
isDebugContext(v8::Local<v8::Context>);
On 2015/05/05 12:41:10, sergeyv wrote:
> ?

This was a leftover from another patch. Removed.

[yurys, 2015-05-05 13:22:57]

There is one issue with this patch. We've got a workaround for backward
compatibility in DOMDebugger[1] which will automatically enable DOM agent if it
is not enabled yet. Pavel, do you think it is time to remove it?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[pfeldman, 2015-05-05 14:00:13]

https://codereview.chromium.org/1129473003/diff/20001/Source/core/inspector/C...
File Source/core/inspector/CodeGeneratorInspector.py (right):

https://codereview.chromium.org/1129473003/diff/20001/Source/core/inspector/C...
Source/core/inspector/CodeGeneratorInspector.py:1729: agent_enablement_check +=
"        reportProtocolError(callId, ServerError, error);\n"
- This is not a server error, this is rather illegal request.
- Also, we should not add logic into this giant generator, but should rather
remove this logic. Otherwise, you are introducing a huge indirection that is
going to be hard to understand / maintain. Why can't we simply check for agent
being enabled wherever it is necessary to be enabled as we agreed offline?

[yurys, 2015-05-05 14:17:07]

Pavel, what are your thoughts re comment #6 ?

On 2015/05/05 14:00:13, pfeldman_ooo_may11 wrote:
>
https://codereview.chromium.org/1129473003/diff/20001/Source/core/inspector/C...
> File Source/core/inspector/CodeGeneratorInspector.py (right):
> 
>
https://codereview.chromium.org/1129473003/diff/20001/Source/core/inspector/C...
> Source/core/inspector/CodeGeneratorInspector.py:1729: agent_enablement_check
+=
> "        reportProtocolError(callId, ServerError, error);\n"
> - This is not a server error, this is rather illegal request.
All agent errors are sent as server error, see
InspectorBackendDispatcherImpl::sendResponse. In other words if it was
dispatched in one of the command handlers its type would be server error not
illegal request (the command implementation cannot affect error type).

> - Also, we should not add logic into this giant generator, but should rather
> remove this logic. Otherwise, you are introducing a huge indirection that is
> going to be hard to understand / maintain. Why can't we simply check for agent
> being enabled wherever it is necessary to be enabled as we agreed offline?

I don't remember us agreeing on polluting code with enablement checks all over
the place or coming to another conclusion that would make everyone happy. I
agree though that having this check in generated dispatcher introduces unobvious
indirection so I don't mind manually checking this in every agent. Unfortunately
such check is very easy to forget about when adding new methods.

[pfeldman, 2015-05-05 14:19:26]

> I don't remember us agreeing on polluting code with enablement checks all over
> the place or coming to another conclusion that would make everyone happy. I
> agree though that having this check in generated dispatcher introduces
unobvious
> indirection so I don't mind manually checking this in every agent.
Unfortunately
> such check is very easy to forget about when adding new methods.

Can we say test it in the ScriptDebugger accessor and fail if it is not there?

(As for DOMDebugger, I think it is fine to fail unless DOM is enabled.)

[yurys, 2015-05-06 08:34:38]

Reverted changed to the code generator. Added manual checks. PTAL.

[sergeyv, 2015-05-06 10:04:41]

lgtm

[yurys, 2015-05-06 10:47:29]

Runtime.getProperties now will fail on attempt to get object's internal
properties if debugger agent is disabled. Alternatively we could silently skip
internal properties collection if ScriptDebugServer is disabled. PTAL

[yurys, 2015-05-06 13:05:56]

The CQ bit was checked by yurys@chromium.org

[yurys, 2015-05-06 13:05:57]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyv@chromium.org
Link to the patchset: https://codereview.chromium.org/1129473003/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-05-06 13:06:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129473003/80001

[yurys, 2015-05-06 13:06:04]

The CQ bit was unchecked by yurys@chromium.org

[yurys, 2015-05-06 13:06:12]

The CQ bit was checked by yurys@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-06 13:06:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129473003/80001

[pfeldman, 2015-05-06 13:34:23]

The CQ bit was unchecked by pfeldman@chromium.org

[pfeldman, 2015-05-06 13:36:04]

I don't see how we can explain runtime agent not working to the users. It is a
part of a public protocol, it needs to continue working. Automation like
webdriver will be enabling runtime and evaluating expressions. It does not need
debugger and will not enable it.

[yurys, 2015-05-07 07:17:32]

On 2015/05/06 13:36:04, pfeldman_ooo_may11 wrote:
> I don't see how we can explain runtime agent not working to the users. It is a
> part of a public protocol, it needs to continue working. Automation like
> webdriver will be enabling runtime and evaluating expressions. It does not
need
> debugger and will not enable it.

I've filed a bug on this crbug.com/485451 and added a FIXME. I will keep current
behavior to avoid regressions: we silently won't include internal properties if
debugger is not enabled.

[yurys, 2015-05-07 07:30:32]

The CQ bit was checked by yurys@chromium.org

[yurys, 2015-05-07 07:30:32]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyv@chromium.org
Link to the patchset: https://codereview.chromium.org/1129473003/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-05-07 07:30:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129473003/100001

[yurys, 2015-05-07 10:25:01]

The CQ bit was checked by yurys@chromium.org

[yurys, 2015-05-07 10:25:02]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyv@chromium.org
Link to the patchset: https://codereview.chromium.org/1129473003/#ps120001
(title: "Removed assert from SDS::getInternalProperties")

[commit-bot: I haz the power, 2015-05-07 10:25:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129473003/120001

[commit-bot: I haz the power, 2015-05-07 12:01:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-07 12:01:14]

Failed to apply patch for Source/core/inspector/InspectorDOMDebuggerAgent.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/inspector/InspectorDOMDebuggerAgent.h
  Hunk #1 FAILED at 102.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/inspector/InspectorDOMDebuggerAgent.h.rej

Patch:       Source/core/inspector/InspectorDOMDebuggerAgent.h
Index: Source/core/inspector/InspectorDOMDebuggerAgent.h
diff --git a/Source/core/inspector/InspectorDOMDebuggerAgent.h
b/Source/core/inspector/InspectorDOMDebuggerAgent.h
index
4638127f06320a96b80bb5942fa7ae568c2e8e14..eebb95a31438ac754d3583c802b3c77a4fa1e36f
100644
--- a/Source/core/inspector/InspectorDOMDebuggerAgent.h
+++ b/Source/core/inspector/InspectorDOMDebuggerAgent.h
@@ -102,6 +102,7 @@ public:
 
 private:
     InspectorDOMDebuggerAgent(InspectorDOMAgent*, InspectorDebuggerAgent*);
+    bool checkEnabled(ErrorString*);
 
     void pauseOnNativeEventIfNeeded(PassRefPtr<JSONObject> eventData, bool
synchronous);
     PassRefPtr<JSONObject> preparePauseOnNativeEventData(const String&
eventName, const String* targetName);

[commit-bot: I haz the power, 2015-05-07 12:01:20]

The CQ bit was unchecked by commit-bot@chromium.org

[yurys, 2015-05-07 13:12:30]

The CQ bit was checked by yurys@chromium.org

[yurys, 2015-05-07 13:12:30]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyv@chromium.org
Link to the patchset: https://codereview.chromium.org/1129473003/#ps140001
(title: "Rebased")

[commit-bot: I haz the power, 2015-05-07 13:12:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129473003/140001

[commit-bot: I haz the power, 2015-05-07 14:47:14]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195053

[tasak, 2015-05-08 08:54:47]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1124083003/ by tasak@google.com.

The reason for reverting is: This breaks browser_tests:
WorkerDevToolsSanityTest.InspectSharedWorker.
.

[yurys, 2015-05-08 09:01:02]

On 2015/05/08 08:54:47, tasak wrote:
> A revert of this CL (patchset #8 id:140001) has been created in
> https://codereview.chromium.org/1124083003/ by mailto:tasak@google.com.
> 
> The reason for reverting is: This breaks browser_tests:
> WorkerDevToolsSanityTest.InspectSharedWorker.
> .

@tasak: including a link to the dashboard would be appreciated.

[yurys, 2015-05-08 13:54:21]

Fix for the test is in cq: https://codereview.chromium.org/1132793002/