Bug 156863

Summary: Web Inspector: Source directives lost when using Function constructor repeatedly
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, ggaren, graouts, joepeck, keith_miller, mark.lam, mattbaker, msaboff, nvasilyev, saam, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 156022    
Attachments:
Description Flags
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix none

Joseph Pecoraro
Reported 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.
Attachments
[PATCH] Proposed Fix (10.40 KB, patch)
2016-04-21 13:54 PDT, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
[PATCH] Proposed Fix (15.53 KB, patch)
2016-04-21 21:25 PDT, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2016-04-21 13:26:27 PDT
<rdar://problem/25861064>
Joseph Pecoraro
Comment 2 2016-04-21 13:54:53 PDT
Created attachment 276954 [details] [PATCH] Proposed Fix
Geoffrey Garen
Comment 3 2016-04-21 14:35:13 PDT
Comment on attachment 276954 [details] [PATCH] Proposed Fix What about cached non-function code, like program code?
Joseph Pecoraro
Comment 4 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.
Joseph Pecoraro
Comment 5 2016-04-21 16:23:17 PDT
> I'll investigate this a bit further. The TDZ variable here was "this", heh.
Joseph Pecoraro
Comment 6 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.
Joseph Pecoraro
Comment 7 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.
Joseph Pecoraro
Comment 8 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.
Joseph Pecoraro
Comment 9 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.
Joseph Pecoraro
Comment 10 2016-04-21 21:25:41 PDT
Created attachment 277007 [details] [PATCH] Proposed Fix
Geoffrey Garen
Comment 11 2016-04-22 16:52:45 PDT
Comment on attachment 277007 [details] [PATCH] Proposed Fix r=me
WebKit Commit Bot
Comment 12 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>
WebKit Commit Bot
Comment 13 2016-04-22 17:40:26 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.