1092231 - fix traits Lists, broken because of Symbol.iterator changes (uses getOwnPropertyNames but not getOwnPropertySymbols)

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

I'm seeing errors in my Firefox nightly from extensions like so:

  Message: TypeError: this.tabs[Symbol.iterator] is not a function
  Stack:
    _destroyWindowTabTracker@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///[my-profile]/extensions/[e-mail]!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/windows/tabs-firefox.js:89:1

Looking at the code, in lib/sdk/windows/tabs-firefox.js we have:

89     for (let tab of this.tabs)

where this.tabs is:

197 var tabs = TabList({ window: null });

and TabList is defined as:

151 const TabList = List.resolve({ constructor: "_init" }).compose(

and List comes from lib/sdk/deprecated/list.js and has:

119 listOptions[iteratorSymbol] = function* iterator() {

where iteratorSymbol comes from lib/sdk/util/iteration.js and does:

13 const JS_HAS_SYMBOLS = typeof Symbol === "function";
14 exports.iteratorSymbol = JS_HAS_SYMBOLS ? Symbol.iterator : "@@iterator";

so it seems like this should all work.  I'm not sure why it does not.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, I bet I know what's going on.  We have, in list.js:

  const List = Trait.resolve({ toString: null }).compose(listOptions);

where listOptions has the iteratorSymbol thing.  I assume this is eventually the compose defined in sdk/deprecated/traits/core.js which does:

    let keys = getOwnPropertyNames(trait);

and 

  const getOwnPropertyNames = Object.getOwnPropertyNames

which won't include Symbol.iterator in keys, since it's not a property name.  This code probably needs to look at getOwnPropertySymbols too.

Comment 2

[Tomislav Jovanovic :zombie]

> First 100 of 2169 failures shown
https://tbpl.mozilla.org/php/getParsedLog.php?id=51569944&tree=Jetpack

Comment 3

[Tomislav Jovanovic :zombie]

also, changes to util/iteration.js from bug 918828 should be synced:

http://hg.mozilla.org/mozilla-central/rev/621224c58e71

Comment 4

[Tomislav Jovanovic :zombie]

uh, old changeset.. the new one is:

http://hg.mozilla.org/mozilla-central/rev/51a1fa4c521f

Comment 5

[Tomislav Jovanovic :zombie]

(In reply to Tomislav Jovanovic [:zombie] from comment #3)
> also, changes to util/iteration.js from bug 918828 should be synced:

actually, that might not be the best change to take..

the new code is simply checking for presence of Symbol, and since Symbols have been defined in the Nightly builds for some time, but Symbol.iterator was not used for iteration until bug 918828, that code doesn't work in a ~week old Nightly, and could break if bug 918828 gets backed-out (again), or if webcompat forces us to restrict it from riding the trains..

Comment 6

[Tomislav Jovanovic :zombie]

scoping down this to fix tests asap, filed bug 1093032 for followup changes in the rest of the SDK code..

Comment 7

[Tomislav Jovanovic :zombie]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1694

i'm not exactly sure this is the right fix, because of my (admittedly poor) understanding of the traits code, but it seems to work..

Comment 8

[Tomislav Jovanovic :zombie]

(In reply to Tomislav Jovanovic [:zombie] from comment #5)
> actually, that might not be the best change to take..

jorendorff agrees (via irc).

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8515939 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1694

I've made few comments, could you please fix them before we land this ? Otherwise patch looks good.

Comment 10

[Tomislav Jovanovic :zombie]

Comment on attachment 8515939 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1694

re-requesting r? without changes, with explanations given in the comments..

Comment 11

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/5533dc4f5fa7dd1d86991a32c6815a3a6320b6a1
bug 1092231 - fix traits to work with Symbols

https://github.com/mozilla/addon-sdk/commit/c968107e8a1b1c444efb3da3e6b565cffba1b2fb
Merge pull request #1694 from zombie/1092231-symbol-iterator

bug 1092231 - fix traits to work with Symbols, r=@Gozala

Comment 12

[Tomislav Jovanovic :zombie]

r=gozala via irc