Use nsIArray for URL window argument

RESOLVED FIXED in Firefox 52

Status

()

Firefox
General
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

({addon-compat})

unspecified
Firefox 52
addon-compat
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

7 months ago
nsISupportsArray is deprecated. We can just swap in nsIArray for nsISupports array in browser.js and optionally update callers to use nsIArray.
(Assignee)

Comment 1

7 months ago
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
Attachment #8804511 - Flags: review?(mconley)
(Assignee)

Comment 2

7 months ago
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
Attachment #8804514 - Flags: review?(jaws)
(Assignee)

Comment 3

7 months ago
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?
Flags: needinfo?(jorge)
Keywords: addon-compat

Comment 4

7 months ago
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?
Attachment #8804511 - Flags: review?(mconley) → review+

Comment 5

7 months ago
(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.
Flags: needinfo?(tabmix.onemen)
Flags: needinfo?(bugzilla)

Comment 6

7 months ago
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.
Attachment #8804514 - Flags: review?(jaws) → review+

Comment 7

7 months ago
Thank you for the info, I will update Tab mix code according to comment 3
Flags: needinfo?(tabmix.onemen)

Comment 8

7 months ago
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)
Flags: needinfo?(bugzilla)
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?
Flags: needinfo?(jorge)
(Assignee)

Comment 10

7 months ago
(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.
(Assignee)

Comment 11

7 months ago
(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!
(Assignee)

Comment 12

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83a71fa64b72
(Assignee)

Comment 13

7 months ago
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
(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.

Comment 15

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8f6c4c5eb703
https://hg.mozilla.org/mozilla-central/rev/d0f100627ed8
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.