Closed Bug 1453153 Opened 6 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
DOM: Events
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: nika, Assigned: annyG)

References

Details

(Keywords: site-compat)

Attachments

(1 file)

Bug 1453153 - Initial removal of moz* APIs in DataTransfer
59 bytes, text/x-review-board-request
nika
: review+
Details
Priority: -- → P3
Assignee: nika → nobody
Assignee: nobody → agakhokidze
Attachment #8989181 - Flags: review?(enndeakin)
FYI, you'll need a DOM peer review for .webidl changes.
Attachment #8989181 - Flags: review?(enndeakin)
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 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).
Attachment #8989181 - Flags: review?(nika) → review-
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 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?
Attachment #8989181 - Flags: review?(nika)
Attachment #8989181 - Flags: review?(nika)
Attachment #8989181 - Flags: review?(enndeakin)
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.)
Attachment #8989181 - Flags: review?(enndeakin) → review+
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
Flags: needinfo?(enndeakin)
Comment on attachment 8989181 [details]
Bug 1453153 - Initial removal of moz* APIs in DataTransfer

https://reviewboard.mozilla.org/r/254238/#review266820
Attachment #8989181 - Flags: review+
Flags: needinfo?(enndeakin)
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.
Attachment #8989181 - Flags: review?(nika) → review+
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
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/485fdf8e37e8
Initial removal of moz* APIs in DataTransfer, r=enndeakin,nika
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
Flags: needinfo?(annygakhokidze)
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)
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db6e8f0ee6b4
Initial removal of moz* APIs in DataTransfer, r=enndeakin,r=nika
https://hg.mozilla.org/mozilla-central/rev/db6e8f0ee6b4
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1485643
Flags: needinfo?(annygakhokidze)
Blocks: 1666287
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: