Bug 150096 – Web Inspector: JavaScriptCore should parse sourceURL and sourceMappingURL directives

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* SUMMARY
JavaScriptCore should parse sourceURL and sourceMappingURL directives.

Currently Web Inspector runs 1-2 regexes over every script source to scan for "sourceURL" and "sourceMappingURL" directives. This is rather inefficient, and just sampling a few websites (apple, verge, cnn) my Mac Pro saw 100+ scripts required >1ms scans.
 
  Durations(us)                                     
           value  ------------- Distribution ------------- count    
               8 |                                         0        
              16 |@@@@@@@@@@@@@@@@@@@@@@@@@@               2598     
              32 |@@@@@@@@                                 787      
              64 |@@                                       205      
             128 |@                                        60       
             256 |@@                                       178      
             512 |@                                        95       
            1024 |@                                        61       
            2048 |                                         33       
            4096 |                                         6        
            8192 |                                         0        

Though the majority of scans are short, it should be possible to detect these comment directives while parsing script comments and eliminate these scans entirely.

* NOTES
- The rules for parsing sourceURL and sourceMappingURL directives are somewhat defined by older articles:
> https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit#heading=h.lmz475t4mvbx
> http://www.softwareishard.com/blog/firebug/firebug-tip-label-dynamic-scripts-with-sourceurl-directive/
> Syntax of the directive needs to match this regular expression:
> /\/\/[@#]\ssourceURL=\s*(\S*?)\s*$/m;

[Radar WebKit Bug Importer]

<rdar://problem/23095020>

[Joseph Pecoraro]

I have implemented this, I just need to write tests.

[Joseph Pecoraro]

Created attachment 263027 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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

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

> Source/JavaScriptCore/parser/Lexer.cpp:2269
> +        skipWhitespace();

Oop, I should drop this skipWhitespace, as it looks like our old code (and the documentation I linked to) only expects a single whitespace character between "//#" and the directive name.

[Joseph Pecoraro]

Created attachment 263029 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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

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

> Source/JavaScriptCore/parser/Lexer.cpp:1750
> +    append8(stringStart, stringEnd - stringStart);
> +    return String(m_buffer8.data(), m_buffer8.size());

Note to reviewers. It is possible that the sourceMappingURL is a long dataURL. Here we are making a copy, which matches what was done previously, but perhaps there is a better solution that avoids a copy. I'm open the suggestions.

[Geoffrey Garen]

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

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

r=me

>> Source/JavaScriptCore/parser/Lexer.cpp:1750
>> +    return String(m_buffer8.data(), m_buffer8.size());
> 
> Note to reviewers. It is possible that the sourceMappingURL is a long dataURL. Here we are making a copy, which matches what was done previously, but perhaps there is a better solution that avoids a copy. I'm open the suggestions.

This is how we do other strings in the parser. The cost can never exceed the original cost of decoding the input script, so it's probably OK.

Long-term, perhaps the parser should change to use substrings of the original string more aggressively.

> Source/JavaScriptCore/parser/Lexer.cpp:1757
> +    unsigned lengthToCheck = length - 1; // Ignore the ending NUL byte in the string literal.

NULL

> Source/JavaScriptCore/parser/Lexer.h:215
> +    String m_sourceURL;
> +    String m_sourceMappingURL;

These names feel a bit too on point.

How about "m_sourceURLDirective" or something to help show that this is not "the truth" but just a developer setting?

> Source/JavaScriptCore/parser/SourceProvider.h:78
> +        void setSourceURL(const String& sourceURL) { m_sourceURL = sourceURL; }
> +        void setSourceMappingURL(const String& sourceMappingURL) { m_sourceMappingURL = sourceMappingURL; }
>  
>          JS_EXPORT_PRIVATE void getID();
>          Vector<size_t>& lineStarts();
>  
>          String m_url;
> +        String m_sourceURL;
> +        String m_sourceMappingURL;

See above.

[Geoffrey Garen]

Part of why this is slow is that we use the regex interpret instead of the JIT. That's fixable -- but probably the best long term solution is to update the CSS parser and stop scanning at all.

[WebKit Commit Bot]

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

Clearing flags on attachment: 263029

Committed r191135: <http://trac.webkit.org/changeset/191135>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 150197

[Build Bot]

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

Attachment 263029 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/312781

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 263607 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Joseph Pecoraro]

When reading the next token, the Lexer asserts the buffers are empty. So we need to clear the buffer after we use it. Other places do similar work (shrinking the buffer). Since this is straight forward I'm not going to ask of another review!

diff --git a/Source/JavaScriptCore/parser/Lexer.cpp b/Source/JavaScriptCore/parser/Lexer.cpp
index bf13d74..46d0ca1 100644
--- a/Source/JavaScriptCore/parser/Lexer.cpp
+++ b/Source/JavaScriptCore/parser/Lexer.cpp
@@ -1747,7 +1747,9 @@ ALWAYS_INLINE String Lexer<T>::parseCommentDirectiveValue()
         return String();
 
     append8(stringStart, stringEnd - stringStart);
-    return String(m_buffer8.data(), m_buffer8.size());
+    String result = String(m_buffer8.data(), m_buffer8.size());
+    m_buffer8.shrink(0);
+    return result;
 }

[Joseph Pecoraro]

Remanded with ASSERT fix: <http://trac.webkit.org/changeset/191355>