Bug 156863 - Web Inspector: Source directives lost when using Function constructor repeatedly
Summary: Web Inspector: Source directives lost when using Function constructor repeatedly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks: 156022
  Show dependency treegraph
 
Reported: 2016-04-21 13:26 PDT by Joseph Pecoraro
Modified: 2016-04-22 17:40 PDT (History)
13 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (10.40 KB, patch)
2016-04-21 13:54 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (15.53 KB, patch)
2016-04-21 21:25 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 2016-04-21 13:26:02 PDT
* SUMMARY
Source directives lost when using Function constructor repeatedly.

* TEST
<script>
console.log(Function("\n//# sourceURL=test.js\nreturn 1+1")());
</script>

* STEPS TO REPRODUCE
1. Inspect test page
2. Reload
  => notice "test.js" shows up in Resources sidebar in Extra Scripts folder
3. Reload again and again
  => notice "test.js" no longer shows up in Resources sidebar in Extra Scripts folder

* NOTES
From bug 156022. When constructing the executable for `Function(str)` we go through:

    frame #0: JSC::CodeCache::getFunctionExecutableFromGlobalCode
    frame #1: JSC::UnlinkedFunctionExecutable::fromGlobalCode
    frame #2: JSC::FunctionExecutable::fromGlobalCode
    frame #3: JSC::constructFunctionSkippingEvalEnabledCheck
    frame #4: JSC::constructFunction
    frame #5: JSC::constructFunction
    frame #6: JSC::callFunctionConstructor

At which point if we do not have it in the CodeCache we parse it. If we do have this in the code cache, we return the cached unlinked function executable skipping parsing.

By skipping parsing we don't update the SourceProvider with the sourceURL/sourceMappingURL, because we normally do that under JSC::Parser<>::parse.
Comment 1 Radar WebKit Bug Importer 2016-04-21 13:26:27 PDT
<rdar://problem/25861064>
Comment 2 Joseph Pecoraro 2016-04-21 13:54:53 PDT
Created attachment 276954 [details]
[PATCH] Proposed Fix
Comment 3 Geoffrey Garen 2016-04-21 14:35:13 PDT
Comment on attachment 276954 [details]
[PATCH] Proposed Fix

What about cached non-function code, like program code?
Comment 4 Joseph Pecoraro 2016-04-21 16:19:53 PDT
> What about cached non-function code, like program code?

I was unable to get them to reproduce this issue.

The cases I know about are:

   - Program or Module
     - <script> / API typically have a URL which dominates even if a sourceURL
   - Eval
     - eval() may have a sourceURL
   - Function
     - Function() may have a sourceURL
     - HTML inline event listeners <body onload="foo">

Programs, at least from a web page, don't seem to have this issue. This handles `Function()`. I couldn't reproduce the issue with `eval`.

For example:

    <script>
    console.log(eval("\n//# sourceURL=test.js\nconsole.log(1)"));
    console.log(eval("\n//# sourceURL=test.js\nconsole.log(2)"));
    </script>

Most of the time the cache is avoided if breakpoints are on which would cause DebuggerOn, but even with breakpoints disabled this was not cached because of TDZ speculation:

    (lldb) f
    frame #0: 0x0000000104e35678 JavaScriptCore`JSC::UnlinkedEvalCodeBlock* JSC::CodeCache::getGlobalCodeBlock ... at CodeCache.cpp:91
       88  	    // FIXME: We should do something smart for TDZ instead of just disabling caching.
       89  	    // https://bugs.webkit.org/show_bug.cgi?id=154010
       90  	    bool canCache = debuggerMode == DebuggerOff && profilerMode == ProfilerOff && !vm.typeProfiler() && !vm.controlFlowProfiler() && !variablesUnderTDZ->size();
    -> 91  	    if (cache && canCache) {

    (lldb) p canCache
    (bool) $12 = false

    (lldb) p variablesUnderTDZ->size()
    (unsigned int) $17 = 1

I'll investigate this a bit further.
Comment 5 Joseph Pecoraro 2016-04-21 16:23:17 PDT
> I'll investigate this a bit further.

The TDZ variable here was "this", heh.
Comment 6 Joseph Pecoraro 2016-04-21 19:53:39 PDT
(In reply to comment #4)
> > What about cached non-function code, like program code?
> 
> I was unable to get them to reproduce this issue.
> 
> The cases I know about are:
> 
>    - Program or Module
>      - <script> / API typically have a URL which dominates even if a
> sourceURL

We may be seeing this with Programs and <script> in 150009.
Comment 7 Joseph Pecoraro 2016-04-21 20:31:14 PDT
> We may be seeing this with Programs and <script> in 150009.

Well, somehow I'm seeing this in bug 150009, and adding identical code to CodeCache for Programs makes this work. I have had no luck creating a reduction for it yet.
Comment 8 Joseph Pecoraro 2016-04-21 21:06:39 PDT
(In reply to comment #7)
> I have had no luck creating a reduction for it yet.

Nevermind, my reduction was working I was just seeing unexpected results because of special handling in the Web Inspector for <script>s, which will need to be addressed separately.
Comment 9 Joseph Pecoraro 2016-04-21 21:18:04 PDT
(In reply to comment #5)
> > I'll investigate this a bit further.
> 
> The TDZ variable here was "this", heh.

This seems like it could be a legitimate bug though. It seems the eval code cache might always be getting thwarted by `this` even if `this` is not used? I'll ask Saam tomorrow.
Comment 10 Joseph Pecoraro 2016-04-21 21:25:41 PDT
Created attachment 277007 [details]
[PATCH] Proposed Fix
Comment 11 Geoffrey Garen 2016-04-22 16:52:45 PDT
Comment on attachment 277007 [details]
[PATCH] Proposed Fix

r=me
Comment 12 WebKit Commit Bot 2016-04-22 17:40:22 PDT
Comment on attachment 277007 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 277007

Committed r199939: <http://trac.webkit.org/changeset/199939>
Comment 13 WebKit Commit Bot 2016-04-22 17:40:26 PDT
All reviewed patches have been landed.  Closing bug.