Bug 43332 – [JSC] Web Inspector: implement breaking from native callback

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Podivilov]

It should be possible to break javascript execution from a native callback.

[Pavel Feldman]

JSC's ScriptDebugServer::breakProgram() needs implementation.

[Joseph Pecoraro]

The original bug for DOM Breakpoints was:
https://bugs.webkit.org/show_bug.cgi?id=42886

[Joseph Pecoraro]

"This method should emulate stopping on a breakpoint, run the message loop and deliver backtrace to the front-end. Basically repeat the original breakpoint loop routine."

[Timothy Hatcher]

How is this used in the Inspector? (So the people implementing it can test it.)

[Pavel Feldman]

(In reply to comment #4)
> How is this used in the Inspector? (So the people implementing it can test it.)

1. In Settings.js set Preferences.nativeInstrumentationEnabled = true (currently is false).
2. In Elements panel, invoke context menu on some node and enable DOM breakpoints.
3. Do something that makes events fire

Expected: JS is stopped, Scripts panel shown, stack shows JS stack that led to the DOM mutation.

[Peter Wang]

Created attachment 145036 [details]
Patch

[WebKit Review Bot]

Attachment 145036 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Wang]

Created attachment 145039 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 145039 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        No test. No impact on others interface of JSC.

There is a bunch of layout tests in LayouTests/inspector/debugger disabled for JSC because of this bug. You should enable them if the functionality is now fixed.

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:158
> +    m_paused = true;

This is not what we expect this method to do, you should run message loop here until the execution is resumed like in case if a regular JS breakpoint. Probably the following code will work:

m_pauseOnNextStatement = true;
pauseIfNeeded(m_currentCallFrame->dynamicGlobalObject());

> Source/WebCore/bindings/js/ScriptDebugServer.h:88
> +    bool supportsNativeBreakpoints() { return true; }

This method now can be removed from both Source/WebCore/bindings/{js,v8}/ScriptDebugServer.h as it returns the same value.

[Peter Wang]

Created attachment 145547 [details]
Patch

[Konrad Piascik]

Comment on attachment 145547 [details]
Patch

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

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:155
> +    if (isPaused() || !m_currentCallFrame)

please use m_paused like the rest of the class instead of isPaused()

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:121
>  void InspectorDebuggerAgent::supportsNativeBreakpoints(ErrorString*, bool* result)

can we not get rid of this method alltogether?  Is it used for something else?

[Pavel Feldman]

Comment on attachment 145547 [details]
Patch

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

>> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:121
>>  void InspectorDebuggerAgent::supportsNativeBreakpoints(ErrorString*, bool* result)
> 
> can we not get rid of this method alltogether?  Is it used for something else?

Now that we consider it supported by all ports, it should go away along with its declaration in the protocol (Inspector.json) and its usage in the front-end. Make sure inspector people take a look at this change prior to landing once all the usages are removed.

[WebKit Review Bot]

Comment on attachment 145547 [details]
Patch

Attachment 145547 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12900220

New failing tests:
http/tests/media/media-source/video-media-source-event-attributes.html

[WebKit Review Bot]

Created attachment 145583 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Pavel Feldman]

Comment on attachment 145547 [details]
Patch

r- per comments above

[Peter Wang]

Created attachment 145688 [details]
Patch

[Peter Wang]

Yury, Konrad, and Pavel, can we do it by two steps?
Firstly, make sure things of JSC Ok in this bug. Secondly, open a new bug to remove the common code in v8 and inspector.

[Pavel Feldman]

(In reply to comment #17)
> Yury, Konrad, and Pavel, can we do it by two steps?
> Firstly, make sure things of JSC Ok in this bug. Secondly, open a new bug to remove the common code in v8 and inspector.

Sounds good to me, just make sure you file the new bug first and refer to it from the change via // FIXME: <bug link> Remove this method.

[Pavel Feldman]

Comment on attachment 145688 [details]
Patch

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

> Source/WebCore/bindings/js/ScriptDebugServer.h:88
> +    bool supportsNativeBreakpoints() { return true; }

Please put fixme here.

[Peter Wang]

Created attachment 146796 [details]
Patch

[WebKit Review Bot]

Comment on attachment 146796 [details]
Patch

Clearing flags on attachment: 146796

Committed r119962: <http://trac.webkit.org/changeset/119962>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

These tests still fail on Qt:
http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r119968%20%2838549%29/results.html

[Csaba Osztrogonác]

(In reply to comment #23)
> These tests still fail on Qt:
> http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r119968%20%2838549%29/results.html

I made a little digging. This patch only enabled this tests on Qt,
they are still skipped on other platforms:
- wincairo/Skipped
- mac/Skipped
- win/Skipped
- gtk/TestExpectations (but here all inspector tests are skipped because of an another bug)

inspector/debugger/dom-breakpoints.html
----------------------------------------
http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r119973%20%2838553%29/inspector/debugger/dom-breakpoints-pretty-diff.html

The results seems correct, but the platform independent (JSC specific expected file wasn't updated long time ago, only the chromium specific)


inspector/debugger/event-listener-breakpoints.html
---------------------------------------------------
http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r119973%20%2838553%29/inspector/debugger/event-listener-breakpoints-pretty-diff.html

It seems correct, but we need JSC and V8 specific expected files.

inspector/debugger/step-through-event-listeners.html
-----------------------------------------------------
http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r119973%20%2838553%29/inspector/debugger/step-through-event-listeners-pretty-diff.html

It seems correct, but we need JSC and V8 specific expected files.

inspector/debugger/xhr-breakpoints.html
----------------------------------------
http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r119973%20%2838553%29/inspector/debugger/xhr-breakpoints-pretty-diff.html

It is absolutely wrong on Qt.

[Csaba Osztrogonác]

I filed a new bug report to track the failures (because I don't want to confuse anybody with discussing in this bug)- https://bugs.webkit.org/show_bug.cgi?id=88780

[Peter Wang]

(In reply to comment #25)
> I filed a new bug report to track the failures (because I don't want to confuse anybody with discussing in this bug)- https://bugs.webkit.org/show_bug.cgi?id=88780

I've investigated the test cases, it's just because expectation file is old.
I'll resolve https://bugs.webkit.org/show_bug.cgi?id=88780 as soon as possible.

[Yury Semikhatsky]

Now that this bug is resolved we should unskip some of the tests that used to fail because of it, see for example: 

# https://bugs.webkit.org/show_bug.cgi?id=43332
inspector/debugger/dom-breakpoints.html
inspector/debugger/event-listener-breakpoints.html
inspector/debugger/step-through-event-listeners.html
inspector/debugger/xhr-breakpoints.html

http://trac.webkit.org/browser/trunk/LayoutTests/platform/qt/TestExpectations#L247