Closed
Bug 1209777
Opened 9 years ago
Closed 9 years ago
comm-central will undoubtedly break when bug 589199 lands
Categories
(Thunderbird :: General, defect)
Thunderbird
General
Tracking
(thunderbird43 unaffected)
RESOLVED
FIXED
Thunderbird 44.0
Tracking | Status | |
---|---|---|
thunderbird43 | --- | unaffected |
People
(Reporter: shu, Assigned: mkmelin)
References
Details
Attachments
(3 files, 2 obsolete files)
438.63 KB,
patch
|
philip.chee
:
review+
|
Details | Diff | Splinter Review |
81.16 KB,
patch
|
Details | Diff | Splinter Review | |
1020.41 KB,
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
Heads up. When ES6 global lexical scope lands per bug 589199, comm-central is going to break. It means: - global 'let' and 'const' bindings are no longer properties - global 'let' and 'const' bindings subject to TDZ The quick but very very dirty fix is to change all top-level lets and consts to vars. Otherwise, it needs to be done on a case by case basis. Look for TypeErrors complaining about things being undefined. Look at the line, and try to figure out if it's accessing a global let or const via a property access. Expose such properties or rewrite the code.
Assignee | ||
Comment 1•9 years ago
|
||
Let's make all top level const/let in comm-central vars instead. Just sent to try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=65d52912b7d8 Patch created running find mail/ mailnews/ calendar/ chat/ im/ suite/ testing/ -type f -iname *.js* -exec sed -i -E 's/^(const|let) /var /g' {} \;
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #8669171 -
Flags: review?(philipp)
Attachment #8669171 -
Flags: review?(neil)
Attachment #8669171 -
Flags: review?(mconley)
Attachment #8669171 -
Flags: review?(aleth)
Attachment #8669171 -
Flags: review?(Pidgeot18)
Comment 2•9 years ago
|
||
My god, this patch is humongous. Is it at all possible to break it up by folder so we can better divide the work?
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Comment 3•9 years ago
|
||
I suppose, though there's enough changes that maybe it's better to sanitycheck the script, and spot check + ok the approach. That's what I did myself:) Nobody want's to go though 1.3M of patch even if I divide it up in a few pieces. (You can also easily run the script yourself for the dir you review.)
Flags: needinfo?(mkmelin+mozilla)
Comment 4•9 years ago
|
||
Comment on attachment 8669171 [details] [diff] [review] bug1209777_const_let_global_c-c.patch Review of attachment 8669171 [details] [diff] [review]: ----------------------------------------------------------------- While almost all of the const changes are unnecessary (and probably many of the let ones as well), I don't think it's particularly worth taking the time right now to figure out which ones are which. Instead, we might as well slowly clean them up as we touch these files again.
Attachment #8669171 -
Flags: review?(Pidgeot18) → review+
Comment 5•9 years ago
|
||
Comment on attachment 8669171 [details] [diff] [review] bug1209777_const_let_global_c-c.patch Review of attachment 8669171 [details] [diff] [review]: ----------------------------------------------------------------- Had to take a few moments to grok what sed was doing there. I suppose if somebody's done some funky formatting where the top-level let/const isn't at the start of the line, I guess we're going to get burned by this... Well, I guess we'll just find out.
Attachment #8669171 -
Flags: review?(mconley) → review+
Comment 6•9 years ago
|
||
Comment on attachment 8669171 [details] [diff] [review] bug1209777_const_let_global_c-c.patch Review of attachment 8669171 [details] [diff] [review]: ----------------------------------------------------------------- Ok, fine with me. I'm not particularly happy about changing toplevel const, but on the other hand this is the safest option. Can you explain in detail in which cases toplevel const would fail?
Attachment #8669171 -
Flags: review?(philipp) → review+
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Philipp Kewisch [:Fallen] from comment #6) > Ok, fine with me. I'm not particularly happy about changing toplevel const, > but on the other hand this is the safest option. Can you explain in detail > in which cases toplevel const would fail? I'll refer to shu.
Flags: needinfo?(shu)
Comment 8•9 years ago
|
||
(In reply to Philipp Kewisch [:Fallen] from comment #6) > Ok, fine with me. I'm not particularly happy about changing toplevel const, > but on the other hand this is the safest option. Can you explain in detail > in which cases toplevel const would fail? I agree. Would it be possible to get away with doing this only for const EXPORTED_SYMBOLS = ... or are there likely to be others that cause failures?
Comment 9•9 years ago
|
||
(In reply to aleth [:aleth] from comment #8) > (In reply to Philipp Kewisch [:Fallen] from comment #6) > > Ok, fine with me. I'm not particularly happy about changing toplevel const, > > but on the other hand this is the safest option. Can you explain in detail > > in which cases toplevel const would fail? > > I agree. Would it be possible to get away with doing this only for > const EXPORTED_SYMBOLS = ... > or are there likely to be others that cause failures? If you export something in EXPORTED_SYMBOLS, that needs to be a const. I know maild.js for sure does that right now. The other scenario is whenever someone this to refer to the global object.
Flags: needinfo?(shu)
Comment 10•9 years ago
|
||
Comment on attachment 8669171 [details] [diff] [review] bug1209777_const_let_global_c-c.patch Review of attachment 8669171 [details] [diff] [review]: ----------------------------------------------------------------- > If you export something in EXPORTED_SYMBOLS, that needs to be a const. I > know maild.js for sure does that right now. The other scenario is whenever > someone this to refer to the global object. ::: chat/modules/socket.jsm @@ +74,5 @@ > * keep shifting the first element off and calling as setTimeout for the > * desired flood time?). > */ > > +var EXPORTED_SYMBOLS = ["Socket"]; In that case this shouldn't be in the patch? Or at least this.EXPORTED_SYMBOLS =... instead? @@ +79,2 @@ > > +var {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components; It just seems unfortunate to make these... @@ +85,5 @@ > // Network errors see: xpcom/base/nsError.h > +var NS_ERROR_MODULE_NETWORK = 2152398848; > +var NS_ERROR_CONNECTION_REFUSED = NS_ERROR_MODULE_NETWORK + 13; > +var NS_ERROR_NET_TIMEOUT = NS_ERROR_MODULE_NETWORK + 14; > +var NS_ERROR_NET_RESET = NS_ERROR_MODULE_NETWORK + 20; ...and these (just to pick two examples) var instead of const.
Comment 11•9 years ago
|
||
Comment on attachment 8669171 [details] [diff] [review] bug1209777_const_let_global_c-c.patch Review of attachment 8669171 [details] [diff] [review]: ----------------------------------------------------------------- r+ in the interest of a quick bustage fix, but I'd still like to understand the issues from comment 10 better.
Attachment #8669171 -
Flags: review?(aleth) → review+
Comment 12•9 years ago
|
||
Comment on attachment 8669171 [details] [diff] [review] bug1209777_const_let_global_c-c.patch Review of attachment 8669171 [details] [diff] [review]: ----------------------------------------------------------------- Wait, shouldn't we be using the same scripts as m-c to generate this? cf bug 1202902 comment 16-18.
Attachment #8669171 -
Flags: review+ → review-
Comment 13•9 years ago
|
||
See also bug 1202902 comment 38 for needed changes that are not generated by those scripts.
Comment 14•9 years ago
|
||
Output from global-let2var.sh. Someone on Linux will have to run lexical-components-to-var.sh - it's not compatible with the OS X sed.
Comment 15•9 years ago
|
||
(In reply to aleth [:aleth] from comment #10) > > +var EXPORTED_SYMBOLS = ["Socket"]; > > In that case this shouldn't be in the patch? > Or at least this.EXPORTED_SYMBOLS =... instead? It seems m-c was using this.EXPORTED_SYMBOLS everywhere even before these recent patches, so we likely need a separate patch for these.
Comment 16•9 years ago
|
||
I managed to get lexical-components-to-var.sh to run on my Mac, I've combined this with aleth's output above.
Attachment #8671338 -
Attachment is obsolete: true
Comment 17•9 years ago
|
||
This patch replaces 'const EXPORTED_SYMBOLS' with this.EXPORTED_SYMBOLS.
Comment 18•9 years ago
|
||
So if we were to go the m-c route with patches from comment 16 and comment 17, what's still needed is the equivalent of bug 1202902 comment 38. Some of those changes aren't covered by mkmelin's original patch either (e.g. the XBL field change).
Comment 19•9 years ago
|
||
In bug 1212911 and bug 1213018 I only fixed problems that caused JS errors in console after startup and the changes are very few. Of course we should also run the test suite to check other code paths. If someone is unhappy with the big patches here, we could also go that route.
Reporter | ||
Comment 20•9 years ago
|
||
(In reply to Magnus Melin from comment #7) > (In reply to Philipp Kewisch [:Fallen] from comment #6) > > Ok, fine with me. I'm not particularly happy about changing toplevel const, > > but on the other hand this is the safest option. Can you explain in detail > > in which cases toplevel const would fail? > > I'll refer to shu. I missed this NI, sorry. The common case of toplevel consts failing is when they are used as properties on the global scope. People often do this: const { SOME_CONSTANT } = Cu.import("SomeModule.jsm", {}), where SOME_CONSTANT is declared with 'const' inside SomeModule.jsm The hard part is that often, things that don't look like property accesses still are property accesses. For instance, sometimes some JSM or JS file's global scope gets put as the [[Prototype]] of a Sandbox. Code running in that Sandbox then try to access bindings declared with 'const' as bareword identifiers. This used to work, as consts used to be properties, and so prototype lookup on the global object would resolve those bindings. This won't work anymore because consts aren't properties.
Reporter | ||
Comment 21•9 years ago
|
||
(In reply to aleth [:aleth] from comment #17) > Created attachment 8671375 [details] [diff] [review] > Replace const EXPORTED_SYMBOLS > > This patch replaces 'const EXPORTED_SYMBOLS' with this.EXPORTED_SYMBOLS. Actually 'const EXPORTED_SYMBOLS' should be fine. There's special logic in the component loader to check for EXPORTED_SYMBOLS both in the global lexical scope and the global object. The problem is, as I said in comment 20, using lets and consts as properties on the scope. This is done often in tests.
Reporter | ||
Comment 22•9 years ago
|
||
(In reply to aleth [:aleth] from comment #18) > So if we were to go the m-c route with patches from comment 16 and comment > 17, what's still needed is the equivalent of bug 1202902 comment 38. Some of > those changes aren't covered by mkmelin's original patch either (e.g. the > XBL field change). Just for reference, manually fixing the world in m-c took about 3 weeks. It's quite soul-crushing, really.
Updated•9 years ago
|
Attachment #8669171 -
Flags: review-
Assignee | ||
Comment 23•9 years ago
|
||
So what's the path forward here? 1) apply attachment 8671375 [details] [diff] [review] 2) apply attachment 8671373 [details] [diff] [review] 3A) something like attachment 8661040 [details] (find-consts.js). How do you actually run this over a bunch of files? or 3B) for the rest, do my original replacement which will change a lot of things consts likely not needed, but should be safe.
Comment 24•9 years ago
|
||
(In reply to aleth [:aleth] from comment #18) > what's still needed is the equivalent of bug 1202902 comment 38. Some of > those changes aren't covered by mkmelin's original patch either (e.g. the > XBL field change). I took a quick look at the XBL fields and c-c doesn't seem to have any problematic ones.
Comment 25•9 years ago
|
||
(In reply to Magnus Melin from comment #23) > So what's the path forward here? > 1) apply attachment 8671375 [details] [diff] [review] > 2) apply attachment 8671373 [details] [diff] [review] > 3A) something like attachment 8661040 [details] (find-consts.js). How do > you actually run this over a bunch of files? > or > 3B) for the rest, do my original replacement which will change a lot of > things consts likely not needed, but should be safe. Honestly, I don't think we have the time to verify consts manually (there's 4,640 in comm-central, according to dxr). 95% of them are safe, but I already know that, e.g., maild.js has some consts that need changing and I suspect most mozmill tests need changing. To me, it's saner to mass-change const and then have people fix them back to const as we revisit code over time.
Comment 26•9 years ago
|
||
Comment on attachment 8671373 [details] [diff] [review] Output of lexical-components-to-var.sh and global-let-to-var.sh > +++ b/suite/modules/WindowsJumpLists.jsm .... > -let EXPORTED_SYMBOLS = [ > +var EXPORTED_SYMBOLS = [ Use this instead: this.EXPORTED_SYMBOLS = [
Attachment #8671373 -
Flags: review+
Assignee | ||
Comment 27•9 years ago
|
||
Refreshed global replace part, to apply on top of attachment 8671373 [details] [diff] [review] and attachment 8671375 [details] [diff] [review]. So, aleth, you're ok with this? Also someone from seamonkey? (If not, I'll flush the suite/ parts). I'd like to land this soon.
Attachment #8669171 -
Attachment is obsolete: true
Attachment #8669171 -
Flags: review?(neil)
Attachment #8672101 -
Flags: review?(philip.chee)
Attachment #8672101 -
Flags: review?(neil)
Attachment #8672101 -
Flags: review?(aleth)
Comment 28•9 years ago
|
||
Comment on attachment 8672101 [details] [diff] [review] bug1209777_const_let_global_c-c.patch Review of attachment 8672101 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Magnus Melin from comment #27) > Refreshed global replace part, to apply on top of attachment 8671373 [details] [diff] [review] > [details] [diff] [review] and attachment 8671375 [details] [diff] [review]. According to Shu in comment 21, the EXPORTED_SYMBOLS patch is strictly speaking not needed. But I suppose your global const->var swap would change those anyway, so using this.EXPORTED_SYMBOLS like m-c seems preferable. > So, aleth, you're ok with this? Also someone from seamonkey? (If not, I'll > flush the suite/ parts). I'd like to land this soon. Yes, given the current bustage and the soul-crushing aspect of 4640 const's I guess we'll have to live with it. Thanks for the patch!
Attachment #8672101 -
Flags: review?(aleth) → review+
Comment 29•9 years ago
|
||
Comment on attachment 8672101 [details] [diff] [review] bug1209777_const_let_global_c-c.patch As an experiment I updated my SeaMonkey build and it seemed OK except the Mail & Newsgroups window was broken by shared mailnews code. It therefore appears to me that suite doesn't need "fixing".
Attachment #8672101 -
Flags: review?(neil)
Assignee | ||
Updated•9 years ago
|
Attachment #8672101 -
Flags: review?(philip.chee)
Assignee | ||
Comment 30•9 years ago
|
||
https://hg.mozilla.org/comm-central/rev/7a2c243821cc https://hg.mozilla.org/comm-central/rev/d09fd3bc8169 https://hg.mozilla.org/comm-central/rev/78ab64eb195b ->FIXED
status-thunderbird43:
--- → unaffected
Target Milestone: --- → Thunderbird 44.0
Updated•9 years ago
|
Severity: normal → blocker
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Comment 34•9 years ago
|
||
(In reply to Magnus Melin from comment #30) > https://hg.mozilla.org/comm-central/rev/7a2c243821cc > https://hg.mozilla.org/comm-central/rev/d09fd3bc8169 > https://hg.mozilla.org/comm-central/rev/78ab64eb195b > > ->FIXED Did you fail to update the bug status?
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Comment 35•9 years ago
|
||
Indeed! -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: needinfo?(mkmelin+mozilla)
Resolution: --- → FIXED
Comment 38•9 years ago
|
||
(In reply to Magnus Melin from comment #35) > Indeed! -> FIXED Yeah, but the latest comm-central build is 10/8's... Sigh... http://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-central/
Comment 39•9 years ago
|
||
This fix also resolves 1213611 on TBx64 on Win 64x. What can I do to help test side effects?
Assignee | ||
Comment 40•9 years ago
|
||
We always want more people using nightlies and reporting problems they find, especially new ones. (Once builds get going again.)
Comment 41•9 years ago
|
||
I was mistaken. The fix for 1209777 does NOT resolve 1213611. Reinstalled, the 10/13 44.0a1 still has no folder|message panes in the x64 release on Win 7. Only uninstalling it and reinstalling the 10/7 restores the panes. Let me know pr. E-mail when there's a new build, and I'll re-test it to see if the bug's gone.
Comment 42•9 years ago
|
||
(In reply to Helge Skjeveland from comment #41) > the 10/13 44.0a1 Where did you get this? The latest comm-central build is 10/8. Did you build Daily yourself?
Comment 44•9 years ago
|
||
I got it via manual Help | About Daily's Check for Updates button. I've downgraded to the 10/7, and won't upgrade until I see it in the latest-comm-central. BTW: Is there an estimate for when 64-bit TB will be available to download & install via the usual mechanism?
Comment 47•9 years ago
|
||
Updating to the 2015-10-30 45.0a1 has restored panes seeming normal functionality.
You need to log in
before you can comment on or make changes to this bug.
Description
•