1390106 - Drop support for versioned JavaScript unless it is web-exposed

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[Masatoshi Kimura [:emk]]

Since we have control over our chrome scripts now, we can stop supporting versioned scripts there.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Bug 1390106 - Stop using versioned scripts in addon-sdk.

Review commit: https://reviewboard.mozilla.org/r/168230/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168230/

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Bug 1390106 - Stop using versioned scripts in dom.

Review commit: https://reviewboard.mozilla.org/r/168232/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168232/

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: Bug 1390106 - Stop using versioned scripts in js/src.

Review commit: https://reviewboard.mozilla.org/r/168234/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168234/

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: Bug 1390106 - Stop using versioned scripts in js/xpconnect.

Review commit: https://reviewboard.mozilla.org/r/168236/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168236/

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Bug 1390106 - Stop using versioned scripts in tests.

Review commit: https://reviewboard.mozilla.org/r/168238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168238/

Comment 6

[Joel Maher ( :jmaher ) (UTC -8) (PTO'ish until July 5)]

Comment on attachment 8896943 [details]
Bug 1390106 - Stop using versioned scripts in tests.

https://reviewboard.mozilla.org/r/168238/#review173468

thanks for the cleanup!

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8896939 [details]
Bug 1390106 - Stop using versioned scripts in addon-sdk.

https://reviewboard.mozilla.org/r/168230/#review173534

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8896940 [details]
Bug 1390106 - Stop using versioned scripts in dom.

https://reviewboard.mozilla.org/r/168232/#review173602

::: dom/xul/nsXULContentSink.cpp:870
(Diff revision 1)
>  
>                // Get the version string, and ensure that JavaScript supports it.
>                nsAutoString versionName;
>                rv = parser.GetParameter("version", versionName);
>  
>                if (NS_SUCCEEDED(rv)) {

Should we maybe warn to the console in this case so that we can get rid of these version parameters? Or is that going to spam so much as to be counterproductive?

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 8896942 [details]
Bug 1390106 - Stop using versioned scripts in js/xpconnect.

https://reviewboard.mozilla.org/r/168236/#review173632

The unit test is using a different kind of iterator now, but I guess that is okay? Is it not possible to use the other kind of iterator now?

Comment 10

[Masatoshi Kimura [:emk]]

Comment on attachment 8896940 [details]
Bug 1390106 - Stop using versioned scripts in dom.

https://reviewboard.mozilla.org/r/168232/#review173602

> Should we maybe warn to the console in this case so that we can get rid of these version parameters? Or is that going to spam so much as to be counterproductive?

We already removed version parameters from XUL files in our tree. This code is left to make sure we will not re-introduce version parameters.

Comment 11

[Masatoshi Kimura [:emk]]

Comment on attachment 8896942 [details]
Bug 1390106 - Stop using versioned scripts in js/xpconnect.

https://reviewboard.mozilla.org/r/168236/#review173632

Yeah, old __iterator__ does not work well with ES6 generatorss.

Comment 12

[Tooru Fujisawa [:arai]]

Comment on attachment 8896941 [details]
Bug 1390106 - Stop using versioned scripts in js/src.

https://reviewboard.mozilla.org/r/168234/#review173870

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 8896941 [details]
Bug 1390106 - Stop using versioned scripts in js/src.

https://reviewboard.mozilla.org/r/168234/#review174136

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a70102f1f011
Stop using versioned scripts in addon-sdk. r=mossop
https://hg.mozilla.org/integration/autoland/rev/1eb26b701975
Stop using versioned scripts in dom. r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/6f3a0d0bc34d
Stop using versioned scripts in js/src. r=luke
https://hg.mozilla.org/integration/autoland/rev/e943579bb054
Stop using versioned scripts in js/xpconnect. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/92e4bdf39c1f
Stop using versioned scripts in tests. r=jmaher

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a70102f1f011
https://hg.mozilla.org/mozilla-central/rev/1eb26b701975
https://hg.mozilla.org/mozilla-central/rev/6f3a0d0bc34d
https://hg.mozilla.org/mozilla-central/rev/e943579bb054
https://hg.mozilla.org/mozilla-central/rev/92e4bdf39c1f

Comment 17

[Joe Sabash [:JoeS1]]

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1417209#c1
The Thunderbird hangs with many addons
Is there a more graceful way that the goals of this bug can be accomplished without completelt killing TB

Comment 18

[Joe Sabash [:JoeS1]]

Point 5 in that comment seems to be the killer

Comment 19

[:aceman]

The problem in TB is, if any addon contains the javascript;version=* specifier in the <link> tag, TB completely hangs without the main window/UI showing. The app continues and must be killed via task manager.

The last thing we see in the terminal/console (but not Error/Browser console) is:
WARNING: NS_ENSURE_TRUE(JSVersion(mLangVersion) != JSVERSION_UNKNOWN) failed: file /var/SSD/TB-hg/mozilla/dom/xul/nsXULElement.cpp, line 2699

After that the app output is silent.

So the question is, is this behaviour intentional? Or is this something TB specific? Normal user is hosed at this point has no way to see any error. I think the message above is even shown only in a debug build, so not normal releases.

Could the script be properly rejected (not loaded) with an error in the Browser console and the execution would continue? Yes, the addon is dead/hosed, but at least the user could diagnose it (see the error in Browser console).

Comment 20

[Masatoshi Kimura [:emk]]

I intended to ignore MIME types with a version parameter rather than a hung-up. So this is definitely a bug.

Comment 21

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/mozilla-central/diff/1eb26b701975/dom/xul/nsXULContentSink.cpp#l1.40
> +                  version = JSVERSION_UNKNOWN;

This line should have been `isJavaScript = false;` (or `return NS_OK;`).

Comment 22

[:aceman]

I'll try that.

Comment 23

[:aceman]

(In reply to Masatoshi Kimura [:emk] from comment #21)
> https://hg.mozilla.org/mozilla-central/diff/1eb26b701975/dom/xul/
> nsXULContentSink.cpp#l1.40
> > +                  version = JSVERSION_UNKNOWN;
> 
> This line should have been `isJavaScript = false;` (or `return NS_OK;`).

I tried the 'isJavaScript = false;' instead of 'version = JSVERSION_UNKNOWN;'. Starting TB worked fine, main window appeared as normal. Clicking a menuitem from the affected addon (so XUL file from it was loaded properly) caused error in browser console about some of its objects missing (so its js file was not loaded).

So this works great as we expected. Can you make a patch for it? In which bug should it go?

Comment 24

[Masatoshi Kimura [:emk]]

> In which bug should it go?

Maybe bug 1417209?