Last Comment Bug 870361 - Change the symbol for source map pragmas from @ to #
: Change the symbol for source map pragmas from @ to #
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla24
Assigned To: Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-09 07:21 PDT by Eddy Bruel [:ejpbruel]
Modified: 2016-05-23 04:54 PDT (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to be reviewed (5.19 KB, patch)
2013-05-09 07:21 PDT, Eddy Bruel [:ejpbruel]
no flags Details | Diff | Splinter Review
Patch to be reviewed (v2) (9.55 KB, patch)
2013-05-09 08:53 PDT, Eddy Bruel [:ejpbruel]
jorendorff: review+
Details | Diff | Splinter Review
v3 (18.39 KB, patch)
2013-06-03 16:51 PDT, Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
no flags Details | Diff | Splinter Review
v4 (17.58 KB, patch)
2013-06-06 08:37 PDT, Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
jorendorff: review+
Details | Diff | Splinter Review
v4.1 (17.81 KB, patch)
2013-06-07 15:31 PDT, Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
no flags Details | Diff | Splinter Review
v4.2 (17.81 KB, patch)
2013-06-07 15:42 PDT, Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
no flags Details | Diff | Splinter Review

Description User image Eddy Bruel [:ejpbruel] 2013-05-09 07:21:34 PDT
Created attachment 747415 [details] [diff] [review]
Patch to be reviewed

Per the discussion on the source map mailing list
Comment 1 User image Eddy Bruel [:ejpbruel] 2013-05-09 08:53:00 PDT
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.
Comment 2 User image Jason Orendorff [:jorendorff] 2013-05-15 22:46:54 PDT
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 User image Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] 2013-05-16 09:31:27 PDT
Spec has been updated, this can land.
Comment 5 User image Phil Ringnalda (:philor) 2013-05-31 22:23:54 PDT
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 User image Eddy Bruel [:ejpbruel] 2013-05-31 22:28:58 PDT
(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 User image Phil Ringnalda (:philor) 2013-05-31 22:34:52 PDT
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 User image Eddy Bruel [:ejpbruel] 2013-05-31 22:56:42 PDT
(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 User image Phil Ringnalda (:philor) 2013-05-31 23:16:07 PDT
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 User image Eddy Bruel [:ejpbruel] 2013-06-01 16:37:07 PDT
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 User image Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] 2013-06-03 16:51:27 PDT
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
Comment 12 User image Phil Ringnalda (:philor) 2013-06-03 17:51:44 PDT
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 User image Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] 2013-06-03 17:56:31 PDT
Try push: https://tbpl.mozilla.org/?tree=Try&rev=931911388f27
Comment 14 User image Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] 2013-06-06 08:37:39 PDT
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
Comment 15 User image Jason Orendorff [:jorendorff] 2013-06-07 15:18:16 PDT
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 User image Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] 2013-06-07 15:31:24 PDT
Created attachment 759986 [details] [diff] [review]
v4.1

Fixed comment mentioned above!
Comment 17 User image Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] 2013-06-07 15:42:43 PDT
Created attachment 759989 [details] [diff] [review]
v4.2

Rebased on central so that it will apply easier for whoever lands this :)
Comment 18 User image Ryan VanderMeulen [:RyanVM] 2013-06-10 05:50:55 PDT
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 User image Ryan VanderMeulen [:RyanVM] 2013-06-10 12:53:16 PDT
https://hg.mozilla.org/mozilla-central/rev/c85bb7761528

Note You need to log in before you can comment on or make changes to this bug.