Drop support for versioned JavaScript unless it is web-exposed

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: emk, Assigned: emk)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

2 years ago
Since we have control over our chrome scripts now, we can stop supporting versioned scripts there.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8896939 - Flags: review?(dtownsend)
(Assignee)

Updated

2 years ago
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED

Comment 6

2 years ago
mozreview-review
Comment on attachment 8896943 [details]
Bug 1390106 - Stop using versioned scripts in tests.

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

thanks for the cleanup!
Attachment #8896943 - Flags: review?(jmaher) → review+

Comment 7

2 years ago
mozreview-review
Comment on attachment 8896939 [details]
Bug 1390106 - Stop using versioned scripts in addon-sdk.

https://reviewboard.mozilla.org/r/168230/#review173534
Attachment #8896939 - Flags: review?(dtownsend) → review+
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?
Attachment #8896940 - Flags: review?(mrbkap) → review+

Comment 9

2 years ago
mozreview-review
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?
Attachment #8896942 - Flags: review?(continuation) → review+
(Assignee)

Comment 10

2 years ago
mozreview-review-reply
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.
(Assignee)

Comment 11

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-review
Comment on attachment 8896941 [details]
Bug 1390106 - Stop using versioned scripts in js/src.

https://reviewboard.mozilla.org/r/168234/#review173870
Attachment #8896941 - Flags: review?(arai.unmht) → review?(luke)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8896941 [details]
Bug 1390106 - Stop using versioned scripts in js/src.

https://reviewboard.mozilla.org/r/168234/#review174136
Attachment #8896941 - Flags: review?(luke) → review+

Comment 14

2 years ago
Pushed by VYV03354@nifty.ne.jp:
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
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1390687
(Assignee)

Updated

2 years ago
Blocks: 1173883
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
Point 5 in that comment seems to be the killer
Depends on: 1417209

Comment 19

a year ago
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).
Flags: needinfo?(VYV03354)
(Assignee)

Comment 20

a year ago
I intended to ignore MIME types with a version parameter rather than a hung-up. So this is definitely a bug.
Flags: needinfo?(VYV03354)
(Assignee)

Comment 21

a year ago
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

a year ago
I'll try that.

Comment 23

a year ago
(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?
(Assignee)

Comment 24

a year ago
> In which bug should it go?

Maybe bug 1417209?
(Assignee)

Updated

a year ago
Duplicate of this bug: 867617
You need to log in before you can comment on or make changes to this bug.