1453153 - Add a pref to disable the DataTransfer moz*At APIs for content

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement, P3)

Description

[Nika Layzell [:nika] (ni? for response)]

see bug 1345192 comment 1

Comment 1

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1453153 - Initial removal of moz* APIs in DataTransfer

Add a pref to disable the DataTransfer moz*At APIs for content process.

Review commit: https://reviewboard.mozilla.org/r/254238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254238/

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

FYI, you'll need a DOM peer review for .webidl changes.

Comment 3

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254238/diff/1-2/

Comment 4

[Neil Deakin]

Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

https://reviewboard.mozilla.org/r/254238/#review261328

::: commit-message-bc6c5:4
(Diff revision 2)
> +Bug 1453153 - Initial removal of moz* APIs in DataTransfer, r?enndeakin,r?nika
> +
> +Add a pref to disable the DataTransfer moz*At APIs for content process.
> +

This checkin comment doesn't match the patch -- no preference is used here.

::: dom/events/test/test_dragstart.html:276
(Diff revision 2)
>    is(dt.getData("URL"), "", "empty line to start uri-list");
>    dt.setData("text/uri-list", "  http://www.mozilla.org#anchor  ");
>    is(dt.getData("URL"), "http://www.mozilla.org#anchor", "uri-list with spaces and hash");
>  
>    // ensure that setDataAt works the same way
> +  dt = SpecialPowers.wrap(dt);

Do you need to continue to wrap 'dt' in all the other cases after this line?

::: dom/webidl/DataTransfer.webidl:80
(Diff revision 2)
>    /**
>     * Holds a list of the format types of the data that is stored for an item
>     * at the specified index. If the index is not in the range from 0 to
>     * itemCount - 1, an empty string list is returned.
>     */
> -  [Throws, NeedsCallerType, UseCounter]
> +  [Throws, NeedsCallerType, UseCounter, ChromeOnly]

You should also make mozItemCount ChromeOnly. And you might as well remove the UseCounter from all of them as well.

Comment 5

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254238/diff/2-3/

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

https://reviewboard.mozilla.org/r/254238/#review261656

::: dom/events/test/test_dragstart.html:150
(Diff revision 3)
> +  // The above test fails if we have SpecialPowers.wrap(dt).types instead of dt.types
> +  // So wrap with special powers after the test

We can actually probably fix this now. I'm guessing the cause of this is the "contains" hack which, IIRC, was added for XUL addons. https://searchfox.org/mozilla-central/source/dom/webidl/DataTransfer.webidl#19-20.

Perhaps you could file a bug pointing at this comment and suggest removing it?

::: dom/events/test/test_dragstart.html:322
(Diff revision 3)
>    // that the type appears in DataTransfer.types. These calls need to be called
>    // with SpecialPowers.wrap(), as adding and removing non-string/file types to
>    // DataTransfer is chrome-only.

This comment isn't fully accurate anymore.

::: dom/tests/mochitest/general/mochitest.ini
(Diff revision 3)
>  subsuite = clipboard
>  [test_bug1208217.html]
>  [test_bug1313753.html]
>  [test_bug1434273.html]
>  [test_clientRects.html]
> -[test_clipboard_disallowed.html]

Please just switch this test to using setData instead of setDataAt

::: dom/webidl/DataTransfer.webidl:57
(Diff revision 3)
>    void addElement(Element element);
>  
>    /**
>     * The number of items being dragged.
>     */
> -  [UseCounter]
> +  [ChromeOnly]

Can we make this behind a pref rather than [ChromeOnly]? It would be nice to easily be able to reverse this temporarially if we run into issues.

::: dom/webidl/DataTransfer.webidl:80
(Diff revision 3)
>    /**
>     * Holds a list of the format types of the data that is stored for an item
>     * at the specified index. If the index is not in the range from 0 to
>     * itemCount - 1, an empty string list is returned.
>     */
> -  [Throws, NeedsCallerType, UseCounter]
> +  [Throws, NeedsCallerType, ChromeOnly]

ChromeOnly and NeedsCallerType are redundant, as the caller type will always be Chrome if ChromeOnly is passed.

::: dom/webidl/DataTransfer.webidl:97
(Diff revision 3)
>     *
>     * @param format the format to remove
>     * @throws NS_ERROR_DOM_INDEX_SIZE_ERR if index is greater or equal than itemCount
>     * @throws NO_MODIFICATION_ALLOWED_ERR if the item cannot be modified
>     */
> -  [Throws, NeedsSubjectPrincipal, UseCounter]
> +  [Throws, NeedsSubjectPrincipal, ChromeOnly]

NeedsSubjectPrincipal here and below will always be Chromeif ChromeOnly is set (although again it would be nicer if we could make this behind a pref).

Comment 7

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

https://reviewboard.mozilla.org/r/254238/#review261328

> Do you need to continue to wrap 'dt' in all the other cases after this line?

No, I dont! Thank you for catching that, I fixed it now.

Comment 8

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

https://reviewboard.mozilla.org/r/254238/#review261656

> We can actually probably fix this now. I'm guessing the cause of this is the "contains" hack which, IIRC, was added for XUL addons. https://searchfox.org/mozilla-central/source/dom/webidl/DataTransfer.webidl#19-20.
> 
> Perhaps you could file a bug pointing at this comment and suggest removing it?

Should I create a bug after I land this patch, so that I can reference this comment?

> ChromeOnly and NeedsCallerType are redundant, as the caller type will always be Chrome if ChromeOnly is passed.

So if moz APIs are hidden behind a pref instead of ChromeOnly, then I shouldn't remove NeedsCallerType, correct?

> NeedsSubjectPrincipal here and below will always be Chromeif ChromeOnly is set (although again it would be nicer if we could make this behind a pref).

same question as above, if moz APIs are hidden behind a pref instead of ChromeOnly, then I shouldn't remove NeedsSubjectPrincipal, correct?

Comment 9

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254238/diff/3-4/

Comment 10

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254238/diff/4-5/

Comment 11

[Neil Deakin]

Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

https://reviewboard.mozilla.org/r/254238/#review265436

::: modules/libpref/init/all.js:5850
(Diff revision 5)
>  pref("dom.events.asyncClipboard.dataTransfer", false);
>  // Should only be enabled in tests
>  pref("dom.events.testing.asyncClipboard", false);
> +
> +// Expose moz* APIs in DataTransfer
> +pref("dom.datatransfer.moz", true);

Is the intent to disable this preference later? It might be nice to have tests that check the preference when false, that is, ensure that the moz APIs are not available in a non-privileged page.)

Comment 12

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254238/diff/5-6/

Comment 13

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254238/diff/6-7/

Comment 14

[Anny Gakhokidze [:annyG]]

Hi Neil, I have added some tests to change that moz APIs are not available in a non-privileged page. Will you please look over them? Thanls

Comment 15

[Neil Deakin]

Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

https://reviewboard.mozilla.org/r/254238/#review266820

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

https://reviewboard.mozilla.org/r/254238/#review267316

::: dom/base/test/test_bug574596.html:44
(Diff revision 7)
>  function dumpTransfer(dataTransfer,expect) {
> -  dtData = dataTransfer.mozItemCount + "items:\n";
> -  for (var i = 0; i < dataTransfer.mozItemCount; i++) {
> -    var dtTypes = dataTransfer.mozTypesAt(i);
> +  dtData = SpecialPowers.wrap(dataTransfer).mozItemCount + "items:\n";
> +  for (var i = 0; i < SpecialPowers.wrap(dataTransfer).mozItemCount; i++) {
> +    var dtTypes = SpecialPowers.wrap(dataTransfer).mozTypesAt(i);
>      for (var j = 0; j < dtTypes.length; j++) {
> -      var actualData = dataTransfer.mozGetDataAt(dtTypes[j],i)
> +      var actualData = SpecialPowers.wrap(dataTransfer).mozGetDataAt(dtTypes[j],i)

nit: add a 'let dt - SpecialPowers.wrap(dataTransfer)' to the start of this function to avoid so many line changes.

::: dom/events/DataTransfer.h:434
(Diff revision 7)
>                                nsTArray<nsCString>* aResult);
>  
> +  // Returns true if moz* APIs should be exposed (true for chrome code or if
> +  // dom.datatransfer.moz pref is enabled).
> +  // The affected moz* APIs are mozItemCount, mozTypesAt, mozClearDataAt, mozSetDataAt, mozGetDataAt
> +  static bool MozCallsEnabled(JSContext* cx, JSObject* obj);

nit: Can we call this MozAtAPIsEnabled or something like that instead?

::: dom/events/DataTransfer.cpp:43
(Diff revision 7)
>  #include "mozilla/dom/BindingUtils.h"
>  #include "mozilla/dom/OSFileSystem.h"
>  #include "mozilla/dom/Promise.h"
>  #include "nsNetUtil.h"
>  
> +#define MOZ_CALLS_ENABLED_PREF "dom.datatransfer.moz"

super-nit: Can we call this pref 'dom.datatransfer.mozAtAPIs'? - I feel like 'moz' is a touch generic of a pref name.

::: dom/events/test/test_dragstart.html:44
(Diff revision 7)
>  
>    expectError(() => gDataTransfer.setData("text/plain", "Some Text"),
>                "NoModificationAllowedError", "setData when read only");
>    expectError(() => gDataTransfer.clearData("text/plain"),
>                "NoModificationAllowedError", "clearData when read only");
> -  expectError(() => gDataTransfer.mozSetDataAt("text/plain", "Some Text", 0),
> +  expectError(() => SpecialPowers.wrap(gDataTransfer).mozSetDataAt("text/plain", "Some Text", 0),

This call is practically identical to the one above for 'setData'... :-/

We can probably just kill this test and the one below it.

::: dom/events/test/test_dragstart.html:150
(Diff revision 7)
> +  // The above test fails if we have SpecialPowers.wrap(dt).types instead of dt.types
> +  // So wrap with special powers after the test
> +  dt = SpecialPowers.wrap(dt);

Please add a note of why (Chrome consumers get the 'ReturnValueNeedsContainsHack')

::: modules/libpref/init/all.js:5861
(Diff revision 7)
> +
> +// Expose moz* APIs in DataTransfer
> +pref("dom.datatransfer.moz", true);

I think I'd like to turn this on for nightly builds (using #ifdef NIGHTLY_BUILD to guard the pref setting).

In release builds it's probably best to be cautious and leave the moz* APIs enabled for now.

Comment 17

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254238/diff/7-8/

Comment 18

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 13 changes to 13 files
remote: 
remote: ******************************* ERROR *******************************
remote: Changeset 3823950c634d alters WebIDL file(s) without DOM peer review:
remote: dom/webidl/DataTransfer.webidl
remote: 
remote: Please, request review from either:
remote:   - Andrea Marchesini (:baku)
remote:   - Andrew McCreight (:mccr8)
remote:   - Ben Kelly (:bkelly)
remote:   - Blake Kaplan (:mrbkap)
remote:   - Bobby Holley (:bholley)
remote:   - Boris Zbarsky (:bz)
remote:   - Ehsan Akhgari (:ehsan)
remote:   - Henri Sivonen (:hsivonen)
remote:   - Kyle Machulis (:qdot)
remote:   - Nika Layzell (:mystor)
remote:   - Olli Pettay (:smaug)
remote:   - Peter Van der Beken (:peterv)
remote: *********************************************************************
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.mozhooks hook failed
abort: push failed on remote

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/485fdf8e37e8
Initial removal of moz* APIs in DataTransfer, r=enndeakin,nika

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset 485fdf8e37e8 (Bug 1453153) for clipboard failures on test_findbar.xul  

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=485fdf8e37e801c10fa32190481d8c1f3f7310b3

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd5d7bcee04b97d2a4b1b87e301105c11b34cc7

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=193382908&repo=mozilla-inbound&lineNumber=1335

23:13:03     INFO - TEST-PASS | toolkit/content/tests/chrome/test_findbar.xul | testFindbarSelection: find field is not focused: button 
23:13:03     INFO - TEST-PASS | toolkit/content/tests/chrome/test_findbar.xul | Incorrect selection in testFindbarSelection: button. Selection:  
23:13:03     INFO - TEST-PASS | toolkit/content/tests/chrome/test_findbar.xul | Failed to close findbar after testFindbarSelection 
23:13:03     INFO - Buffered messages finished
23:13:03     INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_findbar.xul | drop on findbar - got "", expected "Rabbits"
23:13:03     INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
23:13:03     INFO - testDrop@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_window.xul:204:7
23:13:03     INFO - onDocumentLoaded@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_window.xul:123:9
23:13:03     INFO - async*startTestWithBrowser@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_window.xul:96:13
23:13:03     INFO - async*onLoad/<@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_window.xul:75:17
23:13:03     INFO - async*onLoad@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_window.xul:66:14
23:13:03     INFO - onload@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_window.xul:1:1
23:13:03     INFO - TEST-PASS | toolkit/content/tests/chrome/test_findbar.xul | testQuickFindLink: failed to open findbar

Comment 21

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254238/diff/8-9/

Comment 22

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

https://reviewboard.mozilla.org/r/254238/#review269666

::: testing/mochitest/tests/SimpleTest/EventUtils.js:2587
(Diff revisions 8 - 9)
>    function fillDrag(event) {
>      if (aDragData) {
>        for (var i = 0; i < aDragData.length; i++) {
>          var item = aDragData[i];
>          for (var j = 0; j < item.length; j++) {
> -          SpecialPowers.wrap(event.dataTransfer).mozSetDataAt(item[j].type, item[j].data, i);
> +          _EU_maybeWrap(event.dataTransfer).mozSetDataAt(item[j].type, item[j].data, i);

this is the only actual difference (the rest are a result of rebase)

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db6e8f0ee6b4
Initial removal of moz* APIs in DataTransfer, r=enndeakin,r=nika

Comment 24

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/db6e8f0ee6b4

Comment 25

[Kohei Yoshino]

Posted a site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/non-standard-datatransfer-apis-have-been-deprecated/