Change the symbol for source map pragmas from @ to #

RESOLVED FIXED in mozilla24

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: ejpbruel, Assigned: fitzgen)

Tracking

({dev-doc-complete})

unspecified
mozilla24
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 747415 [details] [diff] [review]
Patch to be reviewed

Per the discussion on the source map mailing list
Attachment #747415 - Flags: review?(jorendorff)
(Reporter)

Comment 1

4 years ago
Created attachment 747462 [details] [diff] [review]
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.
Attachment #747415 - Attachment is obsolete: true
Attachment #747415 - Flags: review?(jorendorff)
Attachment #747462 - Flags: review?(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"
Attachment #747462 - Flags: review?(jorendorff) → review+
Spec has been updated, this can land.
(Reporter)

Comment 4

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc37773787ce
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
(Reporter)

Comment 6

4 years ago
(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.
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
(Reporter)

Comment 8

4 years ago
(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?
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.
(Reporter)

Comment 10

4 years ago
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!
Created attachment 757711 [details] [diff] [review]
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
Assignee: general → nfitzgerald
Attachment #747462 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #757711 - Flags: review?(philringnalda)
Attachment #757711 - Flags: review?(jorendorff)
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.
Attachment #757711 - Flags: review?(philringnalda)
Try push: https://tbpl.mozilla.org/?tree=Try&rev=931911388f27
Created attachment 759159 [details] [diff] [review]
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
Attachment #757711 - Attachment is obsolete: true
Attachment #757711 - Flags: review?(jorendorff)
Attachment #759159 - Flags: review?(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.
Attachment #759159 - Flags: review?(jorendorff) → review+
Created attachment 759986 [details] [diff] [review]
v4.1

Fixed comment mentioned above!
Attachment #759159 - Attachment is obsolete: true
Keywords: checkin-needed
Created attachment 759989 [details] [diff] [review]
v4.2

Rebased on central so that it will apply easier for whoever lands this :)
Attachment #759986 - Attachment is obsolete: true
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
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c85bb7761528
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

Updated

3 years ago
Keywords: dev-doc-needed
OS: Mac OS X → All
Hardware: x86 → All
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.