850086 - Recognize source URLs inside multiline comments

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eddy Bruel [:ejpbruel]]

To avoid a crashing bug in IE, several JavaScript transpilers (i.e. Coffeescript) wrap single line comments containing a source mapping URL inside a multiline comment to avoid a crashing bug in IE. We need to be able to handle this case.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Proposed patch

Hey Jason, could you take a look at this?

Btw, I owe you a review. I promise to look at it tomorrow.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 723749 [details] [diff] [review]
Proposed patch

Review of attachment 723749 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/debug/Script-sourceMapURL.js
@@ +37,2 @@
>  assertEq(getSourceMapURL(), 'file:///var/quux.js.map');
>  

Additionally, we should have a test for

    /*\n@sourceMappingURL=file:///baz/bar/foo.js.map\n*/

Because all the transpilers generate the comments with newlines before and after the comment pragma.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 723749 [details] [diff] [review]
Proposed patch

Review of attachment 723749 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/TokenStream.cpp
@@ +764,2 @@
>  {
>      /* Match comments of the form "//@ sourceMappingURL=<url>" */

I think you should probably give an example of the multiline form (forms, assuming Nick's form should be supported too) here as well -- I had to look at the test to figure out what the actual hackaround comment syntax was.  I'd move the IE-hackaround comment's stuff up here as well -- explain at the same place examples are given.

@@ +768,3 @@
>      int32_t c;
>  
> +    if (peekChars(18, peeked) && CharsMatch(peeked, " sourceMappingURL=")) {

Hmm, we probably should have some sort of helper to combine these.  And throw some template niceness at it to get rid of that truly awful 18/19 here!  Gag.

Comment 4

[Eddy Bruel [:ejpbruel]]

Comment on attachment 723749 [details] [diff] [review]
Proposed patch

I'd like to get a quick review for this, but I understand that Jason is currently unavailable, so I'm reassigning the review request to Waldo.

Comment 5

[Eddy Bruel [:ejpbruel]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> Comment on attachment 723749 [details] [diff] [review]
> Proposed patch
> 
> Review of attachment 723749 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/TokenStream.cpp
> @@ +764,2 @@
> >  {
> >      /* Match comments of the form "//@ sourceMappingURL=<url>" */
> 
> I think you should probably give an example of the multiline form (forms,
> assuming Nick's form should be supported too) here as well -- I had to look
> at the test to figure out what the actual hackaround comment syntax was. 
> I'd move the IE-hackaround comment's stuff up here as well -- explain at the
> same place examples are given.

Agreed

> 
> @@ +768,3 @@
> >      int32_t c;
> >  
> > +    if (peekChars(18, peeked) && CharsMatch(peeked, " sourceMappingURL=")) {
> 
> Hmm, we probably should have some sort of helper to combine these.  And
> throw some template niceness at it to get rid of that truly awful 18/19
> here!  Gag.

How about creating a helper function that takes the string length as a template parameter? (I don't think we can do anything as smart as getting the length of a string literal in a template argument, can we?)

Comment 6

[Till Schneidereit [:till]]

Comment on attachment 723749 [details] [diff] [review]
Proposed patch

Review of attachment 723749 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with Nick's and Waldo's first comment addressed.

I think introducing helpers and templification are out of scope for this bug, but maybe you could file a new one for them?

Nice optimization with extracting the "@" detection, btw.

Comment 7

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to land

Comment 8

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to land

Comment 9

[Till Schneidereit [:till]]

Comment on attachment 724040 [details] [diff] [review]
Patch to land

https://hg.mozilla.org/integration/mozilla-inbound/rev/a073d9c49425

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a073d9c49425