Closed Bug 1390106 Opened 3 years ago Closed 3 years ago

Drop support for versioned JavaScript unless it is web-exposed

Categories

(Core :: General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: emk, Assigned: emk)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Since we have control over our chrome scripts now, we can stop supporting versioned scripts there.
Attachment #8896939 - Flags: review?(dtownsend)
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
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 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 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+
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 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 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 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+
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
Duplicate of this bug: 1390687
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
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)
I intended to ignore MIME types with a version parameter rather than a hung-up. So this is definitely a bug.
Flags: needinfo?(VYV03354)
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'll try that.
(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?
> In which bug should it go?

Maybe bug 1417209?
Duplicate of this bug: 867617
You need to log in before you can comment on or make changes to this bug.