Last Comment Bug 1298243 - drag/drop: DataTransfer.types is wrong type
: drag/drop: DataTransfer.types is wrong type
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: 48 Branch
: Unspecified Unspecified
-- normal (vote)
: mozilla52
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 946906 1306220 1309970
Blocks: 1308287 1309296
  Show dependency treegraph
 
Reported: 2016-08-25 18:35 PDT by Piers Haken
Modified: 2017-01-17 05:59 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
part 1. Add some more documentation to DataTransferItemList (1.56 KB, patch)
2016-10-06 23:13 PDT, Boris Zbarsky [:bz] (still a bit busy)
michael: review+
Details | Diff | Splinter Review
part 2. Make the type of a DataTransferItem immutable (1.91 KB, patch)
2016-10-06 23:13 PDT, Boris Zbarsky [:bz] (still a bit busy)
michael: review+
Details | Diff | Splinter Review
part 3. Restrict mutation of a DataTransferItem's kind to codepaths that are actually changing its data (3.10 KB, patch)
2016-10-06 23:13 PDT, Boris Zbarsky [:bz] (still a bit busy)
michael: review+
Details | Diff | Splinter Review
part 4. Drop the pointless ErrorResult from the DataTransferItemList indexed getter (5.34 KB, patch)
2016-10-06 23:13 PDT, Boris Zbarsky [:bz] (still a bit busy)
michael: review+
Details | Diff | Splinter Review
part 5. Notify the DataTransfer whenever its types list changes (6.35 KB, patch)
2016-10-06 23:13 PDT, Boris Zbarsky [:bz] (still a bit busy)
michael: review+
Details | Diff | Splinter Review
part 6. Remove the unused xpidl versions of DataTransfer.types and DataTransfer.getData (3.30 KB, patch)
2016-10-06 23:14 PDT, Boris Zbarsky [:bz] (still a bit busy)
michael: review+
Details | Diff | Splinter Review
part 7. Get rid of uses of .contains() on a DataTransfer's .types in our code, since arrays don't have it (12.22 KB, patch)
2016-10-06 23:15 PDT, Boris Zbarsky [:bz] (still a bit busy)
gijskruitbosch+bugs: review+
Details | Diff | Splinter Review
part 8. Change DataTransfer.types from being a DOMStringList to being a frozen array (9.11 KB, patch)
2016-10-06 23:16 PDT, Boris Zbarsky [:bz] (still a bit busy)
michael: review+
Details | Diff | Splinter Review

Description User image Piers Haken 2016-08-25 18:35:30 PDT
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160817112116

Steps to reproduce:

in firefox, DataTransfer.types is a DOMStringList. it should be a DOMString[].

https://html.spec.whatwg.org/multipage/interaction.html#the-datatransfer-interface
http://w3c.github.io/html/editing.html#the-datatransfer-interface


Actual results:

TypeError: transfer.types.indexOf is not a function


Expected results:

spec compliance
Comment 1 User image Hsin-Yi Tsai [:hsinyi] 2016-08-26 02:23:27 PDT
We currently don't support DOMString[] webidl. From the discussion on bug 923919, seems we weren't going to implement this. Any updates on the plan, Boris?
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2016-08-26 07:42:42 PDT
There is nothing to support; DOMString[] is not valid IDL at all.  See https://github.com/whatwg/html/issues/11 for the year-old spec issue to sort out how it should actually work.

Once the HTML spec gets its act together and decides what the API should actually look like, we will align with that API.
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2016-08-26 07:45:49 PDT
In the meantime, I guess we can look into what other engines do here...  One option is to just implement something and end up with a de-facto standard in the face of the spec not being useful here.
Comment 4 User image Hsin-Yi Tsai [:hsinyi] 2016-08-29 22:37:24 PDT
Hi Stone, would you mind checking comment 3 since you are investigating our drag/drop implementation recently?
Comment 5 User image Ming-Chou Shih [:stone] 2016-09-23 01:27:11 PDT
According to current spec definition [1], the type of DataTransfer#types is DOMString[] (was DOMStringList [2]). In gecko [3], DataTransfer#types uses DOMStringList. Search chromium codes [4], the type of DataTransfer#types is DOMString[].

Tested on different browsers, Edge does not support indexOf but Chrome/Canary does. Looks like we follow old spec [2] and chromium follows current spec. Since the type of DataTransfer#types is going to be changed, maybe we can add a wrapper of DOMStringList with indexOf supported as a short-term solution and replace it when the spec is updated. Or, we leave it as it is and change it when spec is updated.

[1] https://html.spec.whatwg.org/multipage/interaction.html#the-datatransfer-interface
[2] https://www.w3.org/TR/2011/WD-html5-20110113/dnd.html#the-datatransfer-interface
[3] http://searchfox.org/mozilla-central/source/dom/webidl/DataTransfer.webidl#21
[4] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/clipboard/DataTransfer.idl?q=datatransfer.idl&sq=package:chromium&dr&l=40
Comment 6 User image Boris Zbarsky [:bz] (still a bit busy) 2016-09-23 01:52:55 PDT
> Search chromium codes [4], the type of DataTransfer#types is DOMString[]

Right, but what does DOMString[] actually map to in Chrome?  They certainly don't implement the old IDL DOMString[] type.  Does it just return a js Array?  Something else?  That's what needs checking.

> Or, we leave it as it is and change it when spec is updated.

The goal here is to figure out what the spec should be updated _to_ and tell Anne, then implement it.

What does Safari return?  I can check that given steps to reproduce, but this bug has no steps to reproduce...
Comment 7 User image Ming-Chou Shih [:stone] 2016-09-23 02:11:47 PDT
I'm sure but looks like the return type of chromium[1] and webkit[2] is Vector<String>. Trying to make sure it is. [3] is the sample page I used with some manually added console logs for testing.

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/clipboard/DataTransfer.cpp?q=DataTransfer.cp&sq=package:chromium&l=187
[2] https://github.com/WebKit/webkit/blob/master/Source/WebCore/dom/DataTransfer.cpp#L147
[3] http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_draganddrop
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2016-09-23 03:10:46 PDT
The C++ return type is.  The question is what the JS reflection of that ends up being.

Thanks for the testcase link.  Looks like in Chrome at least the return type is a plain array, and it returns a new object each time.

Spec issue for this is https://github.com/whatwg/html/issues/11 and I'll follow up there.
Comment 9 User image Boris Zbarsky [:bz] (still a bit busy) 2016-09-23 03:19:59 PDT
https://github.com/whatwg/html/issues/11#issuecomment-249155408
Comment 10 User image Domenic Denicola 2016-10-04 13:27:21 PDT
https://github.com/whatwg/html/pull/1860
Comment 11 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-06 23:13:04 PDT
Created attachment 8798728 [details] [diff] [review]
part 1.  Add some more documentation to DataTransferItemList
Comment 12 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-06 23:13:17 PDT
Created attachment 8798730 [details] [diff] [review]
part 2.  Make the type of a DataTransferItem immutable
Comment 13 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-06 23:13:30 PDT
Created attachment 8798731 [details] [diff] [review]
part 3.  Restrict mutation of a DataTransferItem's kind to codepaths that are actually changing its data
Comment 14 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-06 23:13:40 PDT
Created attachment 8798732 [details] [diff] [review]
part 4.  Drop the pointless ErrorResult from the DataTransferItemList indexed getter
Comment 15 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-06 23:13:53 PDT
Created attachment 8798733 [details] [diff] [review]
part 5.  Notify the DataTransfer whenever its types list changes
Comment 16 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-06 23:14:09 PDT
Created attachment 8798734 [details] [diff] [review]
part 6.  Remove the unused xpidl versions of DataTransfer.types and DataTransfer.getData
Comment 17 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-06 23:15:47 PDT
Created attachment 8798735 [details] [diff] [review]
part 7.  Get rid of uses of .contains() on a DataTransfer's .types in our code, since arrays don't have it

Gijs, I figure someone who's a Firefox/Toolkit peer should look at this part.  It'll be folded with part 8 when I land, since DOMStringList doesn't have .includes() on it.
Comment 18 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-06 23:16:06 PDT
Created attachment 8798736 [details] [diff] [review]
part 8.  Change DataTransfer.types from being a DOMStringList to being a frozen array
Comment 19 User image :Gijs (away until Feb 27) 2016-10-07 01:54:02 PDT
Comment on attachment 8798735 [details] [diff] [review]
part 7.  Get rid of uses of .contains() on a DataTransfer's .types in our code, since arrays don't have it

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

rs=me
Comment 20 User image Michael Layzell [:mystor] 2016-10-07 11:17:34 PDT
Comment on attachment 8798731 [details] [diff] [review]
part 3.  Restrict mutation of a DataTransferItem's kind to codepaths that are actually changing its data

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

::: dom/events/DataTransfer.cpp
@@ +1475,5 @@
>                                          nsIPrincipal* aPrincipal)
>  {
>    RefPtr<DataTransferItem> item = new DataTransferItem(this,
> +                                                       NS_LITERAL_STRING(kCustomTypesMime),
> +                                                       DataTransferItem::KIND_STRING);

It would be nice if we could also set the kind correctly in DataTransferItemList::Add() to avoid the extra mutation in SetData().

::: dom/events/DataTransferItem.h
@@ +100,5 @@
>      mPrincipal = aPrincipal;
>    }
>  
>    already_AddRefed<nsIVariant> DataNoSecurityCheck();
>    already_AddRefed<nsIVariant> Data(nsIPrincipal* aPrincipal, ErrorResult& aRv);

I'm not sure if you change this in future patches (as I haven't read them yet), but I'm pretty sure that Data() can also change the Kind of the data, as it calls SetData() internally if it needs to fill in external data.
Comment 21 User image Michael Layzell [:mystor] 2016-10-07 11:20:05 PDT
Comment on attachment 8798732 [details] [diff] [review]
part 4.  Drop the pointless ErrorResult from the DataTransferItemList indexed getter

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

This is great. I think I used to do permissions check in IndexedGetter which is why it was marked as [Throws]. Now that I don't anymore this is much nicer.
Comment 22 User image Michael Layzell [:mystor] 2016-10-07 11:25:43 PDT
Comment on attachment 8798733 [details] [diff] [review]
part 5.  Notify the DataTransfer whenever its types list changes

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

::: dom/events/DataTransferItem.cpp
@@ +230,1 @@
>    }

I really don't like this code path. A bit of me wants to change this codepath to not allow for the Kind() to change when we fill in external data, especially because it means that a file can disappear from mItems after trying to get its value.

This would mean that we would just always produce null in this situation.

I think that should be left to a follow-up however, so this solution is good for now.

::: dom/events/DataTransferItemList.cpp
@@ +444,5 @@
> +  if (item->Kind() == DataTransferItem::KIND_FILE || aIndex == 0) {
> +    mDataTransfer->TypesListMayHaveChanged();
> +    if (!aHidden) {
> +      mItems.AppendElement(item);
> +    }

I feel like we should probably be adding the item to mItems before notifying mDataTransfer so that it sees the correct state when that function is being called. If we're only invalidating our cache it should be fine, but I still would feel more comfortable with this being the other way around.
Comment 23 User image Michael Layzell [:mystor] 2016-10-07 11:26:18 PDT
Comment on attachment 8798734 [details] [diff] [review]
part 6.  Remove the unused xpidl versions of DataTransfer.types and DataTransfer.getData

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

\o/
Comment 24 User image Michael Layzell [:mystor] 2016-10-07 11:35:44 PDT
Comment on attachment 8798736 [details] [diff] [review]
part 8.  Change DataTransfer.types from being a DOMStringList to being a frozen array

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

I think this is generally good, but I'm a bit worried about the correctness of the caching (partially because I don't understand how webidl caching works).

::: dom/events/DataTransfer.cpp
@@ +315,1 @@
>  {

Should we Clear() aTypes? If aTypes is non-empty this function will act super strangely.

@@ +321,5 @@
>    for (uint32_t i = 0; i < items->Length(); i++) {
>      DataTransferItem* item = items->ElementAt(i);
>      MOZ_ASSERT(item);
>  
>      if (item->ChromeOnly() && !nsContentUtils::LegacyIsCallerChromeOrNativeCode()) {

Can we use [SubjectPrincipal] to get rid of this call? Does [SubjectPrincipal] work with [Pure, Cached, Frozen]?

::: dom/webidl/DataTransfer.webidl
@@ +17,5 @@
>    [Throws]
>    void setDragImage(Element image, long x, long y);
>  
> +  [Pure, Cached, Frozen]
> +  readonly attribute sequence<DOMString> types;

Will this handle that Chrome code should see a different types array correctly? I don't want some addon code having looked at dataTransfer.types causing data leakage to content.
Comment 25 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-07 11:43:46 PDT
> It would be nice if we could also set the kind correctly in DataTransferItemList::Add()

Do you mean in DataTransferItemList::AppendNewItem?  Because in Add() we're sometimes mutating an existing item-place, so do in fact needs to change kind under SetData().

For AppendNewItem, I guess I could basically hoist the KindFromData() call out of SetData() or something, but since SetData() needs to do it anyway....  Not sure what general behavior you're after here.

> but I'm pretty sure that Data() can also change the Kind of the data

Indeed.  As you note, it does this via calling SetData() and that SetData() callsite is included in the "must notify the datatransfer" list.  ;)

> I really don't like this code path.

I assume you mean the FillInExternalData() bits (yay splinter's broken quoting)?

I agree that it's ugly and all that.  I'm glad you don't want me to fix it here and now.  ;)

> I feel like we should probably be adding the item to mItems before notifying
> mDataTransfer so that it sees the correct state when that function is being called.

Good catch.  Done.

> Should we Clear() aTypes? If aTypes is non-empty this function will act super strangely.

The bindings will always call it with empty aTypes.

I guess I can add a Clear() call, especially because we have internal consumers.

> Can we use [SubjectPrincipal] to get rid of this call?

Yes, that's bug 1308287.

> Will this handle that Chrome code should see a different types array correctly?

Yes, once bug 946906 is fixed.  That's why it's blocking this bug.
Comment 26 User image Michael Layzell [:mystor] 2016-10-07 11:45:54 PDT
Comment on attachment 8798736 [details] [diff] [review]
part 8.  Change DataTransfer.types from being a DOMStringList to being a frozen array

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

Makes sense, in that case r+
Comment 27 User image Pulsebot 2016-10-10 18:38:42 PDT
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6147b5ffb4a0
part 1.  Add some more documentation to DataTransferItemList.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a4c0703e825
part 2.  Make the type of a DataTransferItem immutable.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/1873ffaafd25
part 3.  Restrict mutation of a DataTransferItem's kind to codepaths that are actually changing its data.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/5725d27a0bf1
part 4.  Drop the pointless ErrorResult from the DataTransferItemList indexed getter.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/d15ec8c5a75f
part 5.  Notify the DataTransfer whenever its types list changes.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/1493789a0ce5
part 6.  Remove the unused xpidl versions of DataTransfer.types and DataTransfer.getData.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/67d40adfa5e1
part 7.  Change DataTransfer.types from being a DOMStringList to being a frozen array.  r=mystor,gijs
Comment 29 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-11 07:19:02 PDT
This will have addon compat impact; lots of addons are doing things like "event.dataTransfer.types.contains".  Unfortunately, there really isn't a good way to keep that working while following the spec.  The relevant addons should change to event.dataTransfer.types.includes if they only need to target current Firefox.  If they need to stay compatible across a range of versions, then something like this:

  var func = event.dataTransfer.types.contains || event.dataTransfer.types.includes;
  func.call(event.dataTransfer.types, args);

will work...  There may be other patterns that end up calling contains() on a .types return value too; searching for "contains(" in addons MXR gives way too much noise to tell.

Jorge, if you think this will cause serious problems, I can try to add some sort of backwards-compat shim in this case (e.g. define "contains" as an alias for "includes" on this one array return value, when called from chrome code).  Please let me know?
Comment 30 User image Frank-Rainer Grahl 2016-10-11 11:39:48 PDT
Do these (and some others) not need to be changed from contains to includes too?

https://dxr.mozilla.org/mozilla-central/source/dom/base/contentAreaDropListener.js#36
https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/EditorEventListener.cpp#942
Comment 31 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-11 11:44:22 PDT
> https://dxr.mozilla.org/mozilla-central/source/dom/base/contentAreaDropListener.js#36

This one does not, because I didn't change the thing returned by mozTypesAt.

> https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/EditorEventListener.cpp#942

This has nothing to do with the JS array includes/contains mess.  Contains() on the C++ type is the right thing to use there; it wouldn't have compiled otherwise.  ;)
Comment 32 User image Frank-Rainer Grahl 2016-10-11 11:48:02 PDT
Sorry you are right. It was cpp not js. Braindead. Thanks for the js explanation.
Comment 33 User image Jorge Villalobos [:jorgev] 2016-10-11 12:57:36 PDT
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #29)
> Jorge, if you think this will cause serious problems, I can try to add some
> sort of backwards-compat shim in this case (e.g. define "contains" as an
> alias for "includes" on this one array return value, when called from chrome
> code).  Please let me know?

It does appear to be used heavily, so a shim would be ideal in this case.
Comment 34 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-13 12:16:15 PDT
OK, filed bug 1309970 on adding such a shim.
Comment 35 User image Kohei Yoshino [:kohei] 2016-12-16 00:22:02 PST
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/datatransfer-types-is-now-domstringlist-instead-of-array/

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