Closed
Bug 1390106
Opened 6 years ago
Closed 6 years ago
Drop support for versioned JavaScript unless it is web-exposed
Categories
(Core :: General, enhancement)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: emk, Assigned: emk)
References
(Blocks 1 open bug)
Details
Attachments
(5 files)
59 bytes,
text/x-review-board-request
|
mossop
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mrbkap
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
luke
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mccr8
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jmaher
:
review+
|
Details |
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•6 years ago
|
Attachment #8896939 -
Flags: review?(dtownsend)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Comment 6•6 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•6 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 8•6 years ago
|
||
mozreview-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•6 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•6 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•6 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•6 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
Updated•6 years ago
|
Attachment #8896941 -
Flags: review?(arai.unmht) → review?(luke)
![]() |
||
Comment 13•6 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•6 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
Comment 16•6 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 17•6 years ago
|
||
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•6 years ago
|
||
Point 5 in that comment seems to be the killer
![]() |
||
Comment 19•6 years 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•6 years 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•6 years 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•6 years ago
|
||
I'll try that.
![]() |
||
Comment 23•6 years 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•6 years ago
|
||
> In which bug should it go? Maybe bug 1417209?
You need to log in
before you can comment on or make changes to this bug.
Description
•