Bug 134785 - Web Inspector: Debugger Pause button does not work
Summary: Web Inspector: Debugger Pause button does not work
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: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-07-09 17:29 PDT by Joseph Pecoraro
Modified: 2014-07-11 14:55 PDT (History)
11 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (9.61 KB, patch)
2014-07-09 19:03 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Bots (9.61 KB, patch)
2014-07-10 11:26 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Bots 2 (9.59 KB, patch)
2014-07-10 13:52 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Bots 3 (10.92 KB, patch)
2014-07-11 11:08 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2014-07-09 17:29:44 PDT
* 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)
...
Comment 1 Radar WebKit Bug Importer 2014-07-09 17:30:36 PDT
<rdar://problem/17616885>
Comment 2 Joseph Pecoraro 2014-07-09 17:36:14 PDT
DebuggerManager.prototype.debuggerDidPause has new code that may automatically resume if the call stack it creates is empty. That may be this.
Comment 3 Joseph Pecoraro 2014-07-09 18:10:55 PDT
(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.
Comment 4 Joseph Pecoraro 2014-07-09 18:19:08 PDT
Yes, awesome! Now this shows anonymous scripts in call frames and we can step through them! Huge improvement.
Comment 5 Joseph Pecoraro 2014-07-09 18:51:07 PDT
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.
Comment 6 Joseph Pecoraro 2014-07-09 19:03:41 PDT
Created attachment 234679 [details]
[PATCH] Proposed Fix
Comment 7 Timothy Hatcher 2014-07-10 08:34:03 PDT
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.
Comment 8 Joseph Pecoraro 2014-07-10 10:58:49 PDT
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.
Comment 9 Joseph Pecoraro 2014-07-10 11:20:08 PDT
  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.
Comment 10 Joseph Pecoraro 2014-07-10 11:26:53 PDT
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.
Comment 11 Timothy Hatcher 2014-07-10 11:28:38 PDT
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?
Comment 12 Joseph Pecoraro 2014-07-10 13:47:58 PDT
(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.
Comment 13 Joseph Pecoraro 2014-07-10 13:52:51 PDT
Created attachment 234724 [details]
[PATCH] For Bots 2
Comment 14 Joseph Pecoraro 2014-07-11 11:08:14 PDT
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.
Comment 15 Joseph Pecoraro 2014-07-11 14:48:37 PDT
Comment on attachment 234769 [details]
[PATCH] For Bots 3

rego tested this patch locally and it worked! I'll land.
Comment 16 Manuel Rego Casasnovas (Holidays - back 1st Sep) 2014-07-11 14:49:10 PDT
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+.
Comment 17 Joseph Pecoraro 2014-07-11 14:55:18 PDT
r171013: <http://trac.webkit.org/changeset/171013>