Closed Bug 850086 Opened 8 years ago Closed 8 years ago

Recognize source URLs inside multiline comments

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: ejpbruel, Assigned: ejpbruel)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Proposed patch (obsolete) — Splinter Review
Hey Jason, could you take a look at this?

Btw, I owe you a review. I promise to look at it tomorrow.
Attachment #723749 - Flags: review?(jorendorff)
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 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 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.
Attachment #723749 - Flags: review?(jorendorff) → review?(jwalden+bmo)
(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 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.
Attachment #723749 - Flags: review?(jwalden+bmo) → review+
Attached patch Patch to land (obsolete) — Splinter Review
Attachment #724037 - Flags: checkin?(tschneidereit)
Attached patch Patch to landSplinter Review
Attachment #724037 - Attachment is obsolete: true
Attachment #724037 - Flags: checkin?(tschneidereit)
Attachment #724040 - Flags: checkin?(tschneidereit)
Attachment #723749 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a073d9c49425
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.