Last Comment Bug 1312901 - Use nsIArray for URL window argument
: Use nsIArray for URL window argument
Status: RESOLVED FIXED
: addon-compat
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 52
Assigned To: Eric Rahm [:erahm]
:
:
Mentors:
Depends on:
Blocks: nuke-nsSupportsArray
  Show dependency treegraph
 
Reported: 2016-10-25 16:53 PDT by Eric Rahm [:erahm]
Modified: 2016-10-29 06:19 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Part 1: Update browser.js URL argument check to use nsIArray (2.36 KB, patch)
2016-10-25 17:02 PDT, Eric Rahm [:erahm]
gijskruitbosch+bugs: review+
Details | Diff | Splinter Review
Part 2: Use nsIArray for URL argument when opening a window (3.04 KB, patch)
2016-10-25 17:05 PDT, Eric Rahm [:erahm]
gijskruitbosch+bugs: review+
Details | Diff | Splinter Review

Description User image Eric Rahm [:erahm] 2016-10-25 16:53:39 PDT
nsISupportsArray is deprecated. We can just swap in nsIArray for nsISupports array in browser.js and optionally update callers to use nsIArray.
Comment 1 User image Eric Rahm [:erahm] 2016-10-25 17:02:08 PDT
Created attachment 8804511 [details] [diff] [review]
Part 1: Update browser.js URL argument check to use nsIArray

nsISupportsArrays now implement the nsIArray interface so this can be swapped out.

Mike, I think you're the right person to review this (Gijs is out), feel free to redirect of course.

MozReview-Commit-ID: 8DAa7pYdt1b
Comment 2 User image Eric Rahm [:erahm] 2016-10-25 17:05:57 PDT
Created attachment 8804514 [details] [diff] [review]
Part 2: Use nsIArray for URL argument when opening a window

This updates the URL argument to use an nsIMutableArray instead of an nsIArray.

This is fine for the core of Firefox as we updated browser.js to expect an
nsIArray but has potential to affect addons. I'll add a follow-up analysis of
that and request feedback from the addons team.

Jared, it looks like you're the right person to review this, but feel free to
redirect.

MozReview-Commit-ID: 45kvKCBYNK6
Comment 3 User image Eric Rahm [:erahm] 2016-10-25 17:28:43 PDT
8 addons appear to use the URL params (searching by "instanceof nsISupportsArray" and auditing the usage):

> 1122-install.rdf - Tab Mix Plus
> 399862-install.rdf - Search Fun and Easy Sidebar
> 399938-install.rdf - Netscape Communicator 4.x Account Settings Migrator
> 565634-install.rdf - BackTrack Tab History
> 647194-install.rdf - Addons Manager
> 681099-install.rdf - Tab Mix Plus
> 722520-install.rdf - Tab Mix Plus
> 724821-install.rdf - Tab Mix Plus

Lest filter out the dupes:
> 1122-install.rdf - Tab Mix Plus
> 399862-install.rdf - Search Fun and Easy Sidebar
> 399938-install.rdf - Netscape Communicator 4.x Account Settings Migrator
> 565634-install.rdf - BackTrack Tab History
> 647194-install.rdf - Addons Manager

And then lets see if I can find them on addons.mozilla.org:
> Tab Mix Plus
> BackTrack Tab History

To make these (backward)compatible they'd have to add support for nsIArray, something like the following for Tab Mix:

>    else if (uriToLoad instanceof Ci.nsIArray) {
>      let count = uriToLoad.length;
>      for (let i = 0; i < count; i++) {
>        let uriString = uriToLoad.queryElementAt(i, Ci.nsISupportsString);
>        urls.push(uriString.data);
>      }
>    }

and for BackTrack:

> - if (customDataReferrerUri instanceof Ci.nsISupportsArray) {
> + if (customDataReferrerUri instanceof Ci.nsISupportsArray ||
> +      customDataReferrerUri instanceof Ci.nsIArray) {

Jorge, what do you think of my assessment? Does it seem reasonable to push this change and just do outreach to the addon devs?
Comment 4 User image :Gijs 2016-10-26 06:53:16 PDT
Comment on attachment 8804511 [details] [diff] [review]
Part 1: Update browser.js URL argument check to use nsIArray

Review of attachment 8804511 [details] [diff] [review]:
-----------------------------------------------------------------

AIUI this has no add-on impact because it only alters consumers, which will still work even if we keep sending nsISupportsArray, no?
Comment 5 User image :Gijs 2016-10-26 06:56:17 PDT
(In reply to Eric Rahm [:erahm] from comment #3)
> And then lets see if I can find them on addons.mozilla.org:
> > Tab Mix Plus
> > BackTrack Tab History
> 
> To make these (backward)compatible they'd have to add support for nsIArray,
> something like the following for Tab Mix:
> 
> >    else if (uriToLoad instanceof Ci.nsIArray) {
> >      let count = uriToLoad.length;
> >      for (let i = 0; i < count; i++) {
> >        let uriString = uriToLoad.queryElementAt(i, Ci.nsISupportsString);
> >        urls.push(uriString.data);
> >      }
> >    }
> 
> and for BackTrack:
> 
> > - if (customDataReferrerUri instanceof Ci.nsISupportsArray) {
> > + if (customDataReferrerUri instanceof Ci.nsISupportsArray ||
> > +      customDataReferrerUri instanceof Ci.nsIArray) {
> 
> Jorge, what do you think of my assessment? Does it seem reasonable to push
> this change and just do outreach to the addon devs?

Both of these devs are on bugzilla, so I'm ni'ing them.
Comment 6 User image :Gijs 2016-10-26 06:58:44 PDT
Comment on attachment 8804514 [details] [diff] [review]
Part 2: Use nsIArray for URL argument when opening a window

Review of attachment 8804514 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: browser/components/extensions/ext-windows.js
@@ +124,2 @@
>              for (let url of createData.url) {
> +              array.appendElement(mkstr(url), /* weak = */ false);

hrmpf. Can you file a bug to make the 'weak' argument optional and default to false? From a JS perspective it feels dumb to have to specify this.
Comment 7 User image tabmix.onemen 2016-10-26 08:00:20 PDT
Thank you for the info, I will update Tab mix code according to comment 3
Comment 8 User image Alex Vallat 2016-10-26 10:02:14 PDT
Having reviewed the code, I do not believe this change will impact BackTrack Tab History.

(As you have drawn my attention to nsISupportsArray being deprecated, though, I will change to using nsIMutableArray instead for the next version anyway - thanks)
Comment 9 User image Jorge Villalobos [:jorgev] 2016-10-26 11:08:55 PDT
It looks like a reasonable change, yes. Echoing what Gijs said, I didn't see any changes that would break add-ons right away. Is it that we want to warn developers in advance that this usage is deprecated?
Comment 10 User image Eric Rahm [:erahm] 2016-10-26 11:37:28 PDT
(In reply to :Gijs Kruitbosch from comment #4)
> Comment on attachment 8804511 [details] [diff] [review]
> Part 1: Update browser.js URL argument check to use nsIArray
> 
> Review of attachment 8804511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> AIUI this has no add-on impact because it only alters consumers, which will
> still work even if we keep sending nsISupportsArray, no?

Correct.

(In reply to :Gijs Kruitbosch from comment #6)
> Comment on attachment 8804514 [details] [diff] [review]
> Part 2: Use nsIArray for URL argument when opening a window
> 
> Review of attachment 8804514 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me
> 
> ::: browser/components/extensions/ext-windows.js
> @@ +124,2 @@
> >              for (let url of createData.url) {
> > +              array.appendElement(mkstr(url), /* weak = */ false);
> 
> hrmpf. Can you file a bug to make the 'weak' argument optional and default
> to false? From a JS perspective it feels dumb to have to specify this.

Agreed, it's been pretty annoying. Filed bug 1313150 for this.
Comment 11 User image Eric Rahm [:erahm] 2016-10-26 11:46:22 PDT
(In reply to Jorge Villalobos [:jorgev] from comment #9)
> It looks like a reasonable change, yes. Echoing what Gijs said, I didn't see
> any changes that would break add-ons right away. Is it that we want to warn
> developers in advance that this usage is deprecated?

I was mainly concerned about whether or not my analysis of the add-ons that use the |window.arguments[0]| param was correct and that it would be okay to break them. But the add-on devs have chimed in so I think we're good!
Comment 13 User image Eric Rahm [:erahm] 2016-10-27 19:09:22 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f6c4c5eb7035be37363cd651aefebd2b8d1c957
Bug 1312901 - Part 1: Update browser.js URL argument check to use nsIArray. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f100627ed89dfaede75f4ae63be078f35d8e0c
Bug 1312901 - Part 2: Use nsIArray for URL argument when opening a window. r=Gijs
Comment 14 User image Kris Maglione [:kmag] 2016-10-28 12:38:24 PDT
(In reply to Jorge Villalobos [:jorgev] from comment #9)
> It looks like a reasonable change, yes. Echoing what Gijs said, I didn't see
> any changes that would break add-ons right away. Is it that we want to warn
> developers in advance that this usage is deprecated?

At this point, we should warn them that any use of nsISupportsArray/"@mozilla.org/supports-array;1" is deprecated.

Note You need to log in before you can comment on or make changes to this bug.