870361 - Change the symbol for source map pragmas from @ to #

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

Per the discussion on the source map mailing list

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed (v2)

Peter van der Zee rightly pointed out that we shouldn't just drop support for the old pragma syntax, but emit a deprecation warning instead.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 747462 [details] [diff] [review]
Patch to be reviewed (v2)

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

::: js/src/frontend/TokenStream.cpp
@@ +804,5 @@
>       * To avoid a crashing bug in IE, several JavaScript transpilers
>       * wrap single line comments containing a source mapping URL
>       * inside a multiline comment to avoid a crashing bug in IE. To
>       * avoid potentially expensive lookahead and backtracking, we
> +     * only check for this case if we encounter an '#' character.

"a '#' character", not "an"

Comment 3

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

Spec has been updated, this can land.

Comment 4

[Eddy Bruel [:ejpbruel]]

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

Comment 5

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6058a619fc99 - devtools/server/tests/unit/test_dbgsocket.js would prefer you not do that without including it, https://tbpl.mozilla.org/php/getParsedLog.php?id=23664182&tree=Mozilla-Inbound

Comment 6

[Eddy Bruel [:ejpbruel]]

(In reply to Phil Ringnalda (:philor) from comment #5)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6058a619fc99 -
> devtools/server/tests/unit/test_dbgsocket.js would prefer you not do that
> without including it,
> https://tbpl.mozilla.org/php/getParsedLog.php?id=23664182&tree=Mozilla-
> Inbound

My apologies. I was not aware of that file.

Comment 7

[Phil Ringnalda (:philor)]

Heh, what really got mad about it, though, was mochitest-1 - between httpd.js and the js component loader each having to emit 50 or so deprecation warnings every time they turn around, the debug mochitest-1 logs overflowed buildbot's maximum size of 50MB, and got truncated - https://tbpl.mozilla.org/php/getParsedLog.php?id=23664098&tree=Mozilla-Inbound

Comment 8

[Eddy Bruel [:ejpbruel]]

(In reply to Phil Ringnalda (:philor) from comment #7)
> Heh, what really got mad about it, though, was mochitest-1 - between
> httpd.js and the js component loader each having to emit 50 or so
> deprecation warnings every time they turn around, the debug mochitest-1 logs
> overflowed buildbot's maximum size of 50MB, and got truncated -
> https://tbpl.mozilla.org/php/getParsedLog.php?id=23664098&tree=Mozilla-
> Inbound

That's kind of a problem. We really do want those deprecation warnings. If we have to change all the code in the tree that currently uses source map pragmas, that kind of defeats the point.

What's the best course of action here?

Comment 9

[Phil Ringnalda (:philor)]

http://mxr.mozilla.org/mozilla-central/source/config/Preprocessor.py#135 and the copy of it in js/config, and you're down to just fixing some devtools tests, pushing to try with -u all and looking at a log for every test type to make sure you haven't left any other glaring instances.

Comment 10

[Eddy Bruel [:ejpbruel]]

Nick, I'm terribly sorry, but I don't have time to land this before I leave on PTO. Could you please push this into production during my absence? Looks like it only involves some changes in JS files, so you should be able to do this on your own. Philor described the steps you need to do in comment 9. Thanks!

Comment 11

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

Attachment: v3

Try server is down, will add a try push when it's back up.

* s/an/a/ that Jason pointed out

* s/getAtSourceMappingURL/getSourceMappingURL/ because there is no longer an "@"

* Moved the reporting of the deprecation warning inside getSourceMappingURL so that it isn't fired off every time we find "//@" and instead only when we find "//@ sourceMappingURL=...". This fixes the billions of warnings being issued because of "//@line" comments

* s/@ sourceMappingURL/# sourceMappingURL/ in the devtools tests

Comment 12

[Phil Ringnalda (:philor)]

Comment on attachment 757711 [details] [diff] [review]
v3

Happy to look at the results of your try push if you wind up with failures you're not sure about, but you're not touching anything I should even pretend to have the right to review.

Comment 13

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

Try push: https://tbpl.mozilla.org/?tree=Try&rev=931911388f27

Comment 14

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

Attachment: v4

Eddy changed the contents of one of the asm.js messages, and after reverting that change the tests are not complaining anymore.

Try push looking good.

https://tbpl.mozilla.org/?tree=Try&rev=a22e0035f1ee

Comment 15

[Jason Orendorff [:jorendorff]]

Comment on attachment 759159 [details] [diff] [review]
v4

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

Use options("werror") and assertThrowsInstanceOf(() => eval(...), SyntaxError) to check for the deprecation warning.

::: js/src/frontend/TokenStream.cpp
@@ +845,4 @@
>       *
>       * To avoid a crashing bug in IE, several JavaScript transpilers
>       * wrap single line comments containing a source mapping URL
>       * inside a multiline comment to avoid a crashing bug in IE. To

(pre-existing nit) "To avoid a crashing bug in IE" appears too many times in this comment. Once is enough.

Comment 16

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

Attachment: v4.1

Fixed comment mentioned above!

Comment 17

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

Attachment: v4.2

Rebased on central so that it will apply easier for whoever lands this :)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Please make sure you have hg configured per the directions below so that all the needed commit information is included in the patches you attach.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Florian Scholz (Open Web Docs)]

Docs:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Deprecated_source_map_pragma
https://developer.mozilla.org/en-US/docs/Tools/Debugger/How_to/Use_a_source_map
https://developer.mozilla.org/en-US/Firefox/Releases/24#Developer_Tools

fwiw, this warning is one of the top warnings emitted and seen in the Firefox console.