As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 1309698 - Remove nsISupportsArray usage from nsIDragService. r=smaug
: Remove nsISupportsArray usage from nsIDragService. r=smaug
Status: RESOLVED FIXED
: addon-compat
Product: Core
Classification: Components
Component: Drag and Drop (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla52
Assigned To: Eric Rahm [:erahm]
:
: Neil Deakin
Mentors:
Depends on: 1310040
Blocks: nuke-nsSupportsArray
  Show dependency treegraph
 
Reported: 2016-10-12 13:05 PDT by Eric Rahm [:erahm]
Modified: 2016-10-19 08:07 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Remove usage of nsISupportsArray from nsIDragService (49.44 KB, patch)
2016-10-12 13:05 PDT, Eric Rahm [:erahm]
no flags Details | Diff | Splinter Review
Remove usage of nsISupportsArray from nsIDragService (49.60 KB, patch)
2016-10-17 18:07 PDT, Eric Rahm [:erahm]
bugs: review+
Details | Diff | Splinter Review

Description User image Eric Rahm [:erahm] 2016-10-12 13:05:23 PDT
|nsISupportsArray| is deprecated. We should be able to update nsIDragService to just use nsIArray instead.
Comment 1 User image Eric Rahm [:erahm] 2016-10-12 13:05:26 PDT
Created attachment 8800403 [details] [diff] [review]
Remove usage of nsISupportsArray from nsIDragService

This switches over to using nsIArray instead of nsISupportsArray. Unfortunately it spread out to quite a few files, but they're all mostly cosmetic changes. Olli, it looks like you're the best reviewer for this, but feel free to redirect/add another reviewer.
Comment 2 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-10-12 13:07:49 PDT
Did you check if addons are using the changed .idl methods?
Comment 4 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-10-12 13:13:37 PDT
This would break many addons, as far as I see.
Comment 5 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-10-12 13:13:51 PDT
https://dxr.mozilla.org/addons/search?q=invokeDragSession&redirect=true
Comment 6 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-10-12 13:23:01 PDT
Comment on attachment 8800403 [details] [diff] [review]
Remove usage of nsISupportsArray from nsIDragService

So I don't think we can do this easily.

Could we make nsSupportsArray to implement also nsIArray and then make the .idl methods to take nsISupports and QI to nsIArray in the method implementations?
Comment 7 User image Eric Rahm [:erahm] 2016-10-12 14:18:10 PDT
(In reply to Olli Pettay [:smaug] from comment #4)
> This would break many addons, as far as I see.

Good point, I neglected to search this one first. Lets see what the add-ons folks think. Of particular interest to me is how many of these add-ons support the latest release, it looks like a lot of instances of the same add-ons (adblock) and/or copy and pasted code.

Kris, what I'm looking at here is transitioning from |nsISupportsArray| to |nsIArray|. |nsISupportsArray| has been deprecated since 2002.
Comment 8 User image Eric Rahm [:erahm] 2016-10-12 16:23:50 PDT
My calculations put it at 114 add-ons (supported or not) reference this method. A naive approach of downloading the 'install.rdf' and grepping for max version came out with a smaller list that indicate they support the latest release, 49, or all versions:

> ./486302-install.rdf - Mouse Gestures Suite - 50
> ./649888-install.rdf - Gmail Manager-community - 50
> ./654084-install.rdf - Outwit Docs - *
> ./672534-install.rdf - Gmail Manager-Community - 50
> ./681516-install.rdf - Gestão de Conteúdos - 50
> ./691264-install.rdf - PasswordMaker - *
> ./701973-install.rdf - DOM Inspector - 49 49
> ./701974-install.rdf - DOM Inspector - 49 49
> ./701976-install.rdf - DOM Inspector - 49 49 49
> ./712774-install.rdf - Planetromeo-Extension vom Basar der Sklaven-Gazette - * *
> ./718669-install.rdf - DOM Inspector - 50
> ./720315-install.rdf - AutoProxy - *
> ./721650-install.rdf - autoproxy Customization - *
> ./723849-install.rdf - AutoProxyTemp - 50
> ./723850-install.rdf - AutoProxyTemp - 50
> ./727671-install.rdf - AutoProxy - *
> ./731119-install.rdf - AutoProxy - 50
> ./736403-install.rdf - DOM Inspector - 51
> ./737398-install.rdf - Gestão de Conteúdos - 51

Now we can see many of those are duplicates in name at least, if we pare that down:

> ./486302-install.rdf - Mouse Gestures Suite - 50
> ./654084-install.rdf - Outwit Docs - *
> ./672534-install.rdf - Gmail Manager-Community - 50
> ./691264-install.rdf - PasswordMaker - *
> ./712774-install.rdf - Planetromeo-Extension vom Basar der Sklaven-Gazette - * *
> ./721650-install.rdf - autoproxy Customization - *
> ./723849-install.rdf - AutoProxyTemp - 50
> ./731119-install.rdf - AutoProxy - 50
> ./736403-install.rdf - DOM Inspector - 51
> ./737398-install.rdf - Gestão de Conteúdos - 51

And now lets see which ones actually show up in searches on addons.mozilla.org:

> "Mouse Gestures Suite": https://addons.mozilla.org/en-US/firefox/addon/mouse-gestures-suite/?src=search
> "Outwit Docs": https://addons.mozilla.org/en-US/firefox/addon/outwit-docs/?src=search
> "Gmail Manager-Community": https://addons.mozilla.org/en-US/firefox/addon/gmail-manager-community/?src=search
> "PasswordMaker": https://addons.mozilla.org/en-US/firefox/addon/passwordmaker/?src=search
> "AutoProxy": https://addons.mozilla.org/en-US/firefox/addon/autoproxy/?src=search
> "DOM Inspector": https://addons.mozilla.org/en-US/firefox/addon/dom-inspector-6622/?src=search

"Gestão de Conteúdos" doesn't show up for me, but seems like it should? Maybe it's a localization thing.

So by my naive estimation 6 installable from addons.mozilla.org add-ons would be affected.
Comment 9 User image Neil Deakin 2016-10-12 16:29:09 PDT
We should be stronger about encouraging addon authors to use the standard dnd api instead. Using the invoke methods directly from js has been unofficially deprecated since 2008. Can you perhaps mark this as such in nsIDragService?
Comment 10 User image Eric Rahm [:erahm] 2016-10-12 16:38:01 PDT
(In reply to Neil Deakin from comment #9)
> We should be stronger about encouraging addon authors to use the standard
> dnd api instead. Using the invoke methods directly from js has been
> unofficially deprecated since 2008. Can you perhaps mark this as such in
> nsIDragService?

Sure, is the whole service deprecated or just the invoke methods?
Comment 11 User image Neil Deakin 2016-10-13 06:15:36 PDT
Just the invoke methods. There isn't an equivalent to the remaining functions currently.
Comment 12 User image Eric Rahm [:erahm] 2016-10-13 11:22:13 PDT
Discussed with Kris, the plan is to make nsISupportsArray implement nsIArray for a transition period of one release cycle giving add-on devs time to update. In the meantime we can transition our interfaces to nsIArray.
Comment 13 User image Eric Rahm [:erahm] 2016-10-17 18:07:50 PDT
Created attachment 8801943 [details] [diff] [review]
Remove usage of nsISupportsArray from nsIDragService

This a rebased version of the original patch that also deprecates the
invokeDrag functions. Once bug 1310040 lands concerns about breaking addons
should be a non-issue.

Olli, do you mind taking another look? Of course I'll hold off on landing
until bug 1310040 lands.
Comment 15 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-10-18 05:19:14 PDT
Comment on attachment 8801943 [details] [diff] [review]
Remove usage of nsISupportsArray from nsIDragService

>@@ -43,18 +43,18 @@ interface nsIDragService : nsISupports
>     * @param  aTransferables - an array of transferables to be dragged
>     * @param  aRegion - a region containing rectangles for cursor feedback, 
>     *            in window coordinates.
>     * @param  aActionType - specified which of copy/move/link are allowed
>     * @param  aContentPolicyType - the contentPolicyType that will be
>     *           passed to the loadInfo when creating a new channel
>     *           (defaults to TYPE_OTHER)
>     */
>-  void invokeDragSession (in nsIDOMNode aDOMNode,
>-                          in nsISupportsArray aTransferables, 
>+  [deprecated] void invokeDragSession (in nsIDOMNode aDOMNode,
>+                          in nsIArray aTransferables, 
So does this still let JS to pass the same nsISupportsArray (which just happens to support also nsIArray) without any extra QI on JS side?
Please make sure to test it works. I think xpconnect does the QI implicitly, but better to test.
Comment 16 User image Neil Deakin 2016-10-18 06:13:58 PDT
Note: the invoke methods are only deprecated from JS callers.
Comment 17 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-10-18 06:20:25 PDT
yeah, indeed. Would be good to add some comment.
Comment 18 User image Kris Maglione [:kmag] 2016-10-18 09:44:39 PDT
(In reply to Olli Pettay [:smaug] from comment #15)
> So does this still let JS to pass the same nsISupportsArray (which just
> happens to support also nsIArray) without any extra QI on JS side?
> Please make sure to test it works. I think xpconnect does the QI implicitly,
> but better to test.

It does implicitly QI, yes. There's a lot of existing code that depends on this.
Comment 19 User image Eric Rahm [:erahm] 2016-10-18 10:23:01 PDT
(In reply to Kris Maglione [:kmag] from comment #18)
> (In reply to Olli Pettay [:smaug] from comment #15)
> > So does this still let JS to pass the same nsISupportsArray (which just
> > happens to support also nsIArray) without any extra QI on JS side?
> > Please make sure to test it works. I think xpconnect does the QI implicitly,
> > but better to test.
> 
> It does implicitly QI, yes. There's a lot of existing code that depends on
> this.

And there's also a test in bug 1310040 making sure that a nsSupportsArray instance can be QI'd to a nsIArray, so we're probably good.
Comment 20 User image Eric Rahm [:erahm] 2016-10-18 10:24:45 PDT
(In reply to Neil Deakin from comment #16)
> Note: the invoke methods are only deprecated from JS callers.

Okay I'll switch to just adding a comment, explicitly marking as deprecated will cause warnings on the native side.
Comment 21 User image Eric Rahm [:erahm] 2016-10-18 11:57:16 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/32b590e82c8ec5251e77451f91f1e3f8b9ccb92f
Bug 1309698 - Remove usage of nsISupportsArray from nsIDragService. r=smaug
Comment 22 User image Pulsebot 2016-10-18 11:57:18 PDT
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32b590e82c8e
Remove usage of nsISupportsArray from nsIDragService. r=smaug
Comment 23 User image Carsten Book [:Tomcat] 2016-10-19 08:07:42 PDT
https://hg.mozilla.org/mozilla-central/rev/32b590e82c8e

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