Bug 134785 – Web Inspector: Debugger Pause button does not work

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* STEPS TO REPRODUCE
1. Inspect <http://apple.com>
2. Open Debugger sidebar
3. Tap Pause debugger button
4. Scroll the page
  => Pause debugger button toggles, but debugger did not pause

* NOTES
- Looks like the frontend automatically resumed, why?
...
[Log] frontend: {"method":"Debugger.pause","id":111} (Main.js, line 573)
[Log] backend: {"result":{},"id":111} (Main.js, line 540)
[Log] backend: {"method":"Debugger.paused","params":{"callFrames":[{"callFrameId":"{\"ordinal\":0,\"injectedScriptId\":3}","functionName":"anonymous","location":{"scriptId":"400","lineNumber":0,"columnNumber":0},"scopeChain":[{"object":{"type":"object","objectId":"{\"injectedScriptId\":3,\"id\":7}","className":"Window","description":"Window"},"type":"local"},{"object":{"type":"object","objectId":"{\"injectedScriptId\":3,\"id\":8}","className":"Window","description":"Window"},"type":"global"}],"this":{"type":"object","objectId":"{\"injectedScriptId\":3,\"id\":9}","className":"Window","description":"Window"}}],"reason":"other"}} (Main.js, line 540)
[Log] frontend: {"method":"Debugger.resume","id":112} (Main.js, line 573)
[Log] backend: {"result":{},"id":112} (Main.js, line 540)
[Log] backend: {"method":"Debugger.resumed"} (Main.js, line 540)
...

[Radar WebKit Bug Importer]

<rdar://problem/17616885>

[Joseph Pecoraro]

DebuggerManager.prototype.debuggerDidPause has new code that may automatically resume if the call stack it creates is empty. That may be this.

[Joseph Pecoraro]

(In reply to comment #2)
> DebuggerManager.prototype.debuggerDidPause has new code that may automatically resume if the call stack it creates is empty. That may be this.

Yes, this is the case.

In this case, the call stack is an anonymous script, which does not have a URL, so we don't create a call frame for it. Details of the anonymous script are:

    js> JSON.stringify(callFramePayload.location)
    "{"scriptId":"31","lineNumber":0,"columnNumber":0}" 

    js> WebInspector.debuggerManager.scriptForIdentifier("31").displayName
    "Anonymous Script 1"

    js> WebInspector.debuggerManager.scriptForIdentifier("31").requestContent(function() { console.log(arguments); })
    [Object , "(function() {if(!s.getPPVid)return;var dh=Math.max(Math.max(s.d.body.scrollHeight,s...", false]

Which correlates to this code on the page: (s_code_h.js)

    s.handlePPVevents = new Function("", "if(!s.getPPVid)return;var dh=Math.max(Math.max(s.d.body.scrollHeight,s...");

So, I think we should be able to show this. We should not skip over scripts without a URL.

[Joseph Pecoraro]

Yes, awesome! Now this shows anonymous scripts in call frames and we can step through them! Huge improvement.

[Joseph Pecoraro]

So, now we will be able to step into this code into the anonymous function:

Test:

>    <script>
>    var f = new Function("\nconsole.log(1);\nconsole.log(2);\nconsole.log(3);\n");
>    function doit() {
>        f();
>    }
>    </script>

Inspector:

    js> doit();
    step into and through f()

A problem right now is InjectedScriptSource.js gets its //#sourceURL comment stripped when it gets minified. I have a fix for adding that comment back when we minify.

[Joseph Pecoraro]

Created attachment 234679 [details]
[PATCH] Proposed Fix

[Timothy Hatcher]

Comment on attachment 234679 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/CMakeLists.txt:902
> +    COMMAND ${PERL_EXECUTABLE} -e 'print "//# sourceURL=__WebInspectorInjectedScript__\n"' > ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InjectedScriptSource.min.js

Is there is a better, shared, place to add this line? I guess the same question applies to the rest of the build logic that is duplicated.

Maybe we can hack jsmin.py to not strip sourceURL? Likely not worth it though…

> Source/WebInspectorUI/ChangeLog:13
> +        Now that we allow anonymous scripts we can step into anonymous

We have always allowed anon scripts. I think this was just an oversight with the skipping code.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:-373
> -        if (!this._callFrames.length) {
> -            this.resume();
> -            return;
> -        }

I wonder how often this legitimately happens still.

[Joseph Pecoraro]

Comment on attachment 234679 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/CMakeLists.txt:902
>> +    COMMAND ${PERL_EXECUTABLE} -e 'print "//# sourceURL=__WebInspectorInjectedScript__\n"' > ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InjectedScriptSource.min.js
> 
> Is there is a better, shared, place to add this line? I guess the same question applies to the rest of the build logic that is duplicated.
> 
> Maybe we can hack jsmin.py to not strip sourceURL? Likely not worth it though…

Correct, I think the idea fix would be to use a minifier that doesn't strip such comments.

But, as a workaround, I felt this was a small enough change that I could just put it here. It only happens for two files.

This seems to break the EFL build though, I'll need to figure that out.

>> Source/WebInspectorUI/ChangeLog:13
>> +        Now that we allow anonymous scripts we can step into anonymous
> 
> We have always allowed anon scripts. I think this was just an oversight with the skipping code.

Yep.

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:-373
>> -        }
> 
> I wonder how often this legitimately happens still.

If it does legitimately happen, we still have a good UI that we show. Something like "No Call Frames" in the sidebar.

Maybe it can happen for eval("console.log(1);\ndebugger;\nconsole.log(2);"). I'll try it.

[Joseph Pecoraro]

  Syntax Warning in cmake code at

    /home/gyuyoung/eflews/WebKit/Source/JavaScriptCore/CMakeLists.txt:902:90

  Argument not separated from preceding token by whitespace.
This warning is for project developers.  Use -Wno-dev to suppress it.

The line is:

> COMMAND ${PERL_EXECUTABLE} -e 'print "//# sourceURL=__WebInspectorInjectedScript__\n"' > ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InjectedScriptSource.min.js

Maybe CMakeLists.txt is using the \n itself instead of letting perl handle it? I'm baffled right now. I just want to append this text to the top of the file, maybe there is a better way. I'll try switching to python.

[Joseph Pecoraro]

Created attachment 234711 [details]
[PATCH] For Bots

See if this fixes GTK / EFL. I switched from `perl -e '...'` to `python -c '...'` and got rid of "\n" characters in the CMake file.

[Timothy Hatcher]

Comment on attachment 234711 [details]
[PATCH] For Bots

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

> Source/JavaScriptCore/CMakeLists.txt:902
> +    COMMAND ${PYTHON_EXECUTABLE} -c 'print "//# sourceURL=__WebInspectorInjectedScript__"' > ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InjectedScriptSource.min.js

Why can't you use echo here too?

[Joseph Pecoraro]

(In reply to comment #11)
> (From update of attachment 234711 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234711&action=review
> 
> > Source/JavaScriptCore/CMakeLists.txt:902
> > +    COMMAND ${PYTHON_EXECUTABLE} -c 'print "//# sourceURL=__WebInspectorInjectedScript__"' > ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InjectedScriptSource.min.js
> 
> Why can't you use echo here too?

I was unsure if I could in CMake. I'll try.

[Joseph Pecoraro]

Created attachment 234724 [details]
[PATCH] For Bots 2

[Joseph Pecoraro]

Created attachment 234769 [details]
[PATCH] For Bots 3

Bots were red all day yesterday so the patch could not be tested. Rebased and trying again.

[Joseph Pecoraro]

Comment on attachment 234769 [details]
[PATCH] For Bots 3

rego tested this patch locally and it worked! I'll land.

[Manuel Rego Casasnovas]

FWIW, the first patch 234679 was breaking the build in GTK+ and causing issues in the EWS bots.

The last one 234769 builds properly in GTK+.

[Joseph Pecoraro]

r171013: <http://trac.webkit.org/changeset/171013>