Closed Bug 1330979 Opened 3 years ago Closed 3 years ago

Drag & drop files no longer works on onedrive.com

Categories

(Core :: DOM: Drag & Drop, defect, major)

51 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 blocking verified
firefox52 + verified
firefox53 + verified

People

(Reporter: pauly, Assigned: Nika)

References

Details

(Keywords: qablocker, regression)

Attachments

(1 file)

[Affected versions]:
- 51b14, 53.0a1 (2017-01-13)

[Affected platforms]:
- all

[Steps to reproduce]:
1. Login to onedrive.com
2. Drag & drop any file in the main view on onedrive to upload it

[Expected result]:
- The upload should start

[Actual result]:
- Nothing happens, the upload doesn't start
Browser console shows:
"XML Parsing Error: no root element found
Location: https://browser.pipe.aria.microsoft.com/Collector/3.0/?qsp=true&content-type=application%2Fbond-compact-binary&client-id=NO_AUTH&sdk-version=ACT-Web-JS-2.5.0&x-apikey=a23e4f242c9c4097a968f28c62633e19-62d0d830-5afd-4df3-8e40-351c8711cf5c-7157
Line Number 1, Column 1:"

[Regression range]:
- recent regression, doesn't reproduce on 50.1.0
Keywords: regression
Michael, one of your patches.
Flags: needinfo?(michael)
[Tracking Requested - why for this release]:
We probably should not ship with this regression as I think OneDrive is a fairly widespread service. 
Neil, Michael, can you take a look ?
Flags: needinfo?(enndeakin)
In our previous code, we would accidentially not perform the correct
security checks when retreiving data from DataTransferItems in some
situations. The regressing patch fixed this behavior and ensured that we
always perform the correct security checks, making any "insecure"
accesses (such as accesses to file data from the OS in DragEnter) would
raise an NS_ERROR_DOM_SECURTY_ERROR. This behavior is not consistent
with Chrome, and thus OneDrive would accidentally trigger an exception
and break its d&d handling logic.

With this patch our behavior for "insecure" accesses to file data from
the OS is more in line with Chrome's, in that we now don't raise an
exception, but instead just produce the value "null" when the data
should not be avaliable yet. From my quick test this fixes the problem
with OneDrive, and should be a fairly trivial patch to uplift to Beta,
with very little risk.

This patch doesn't include a test, as Drag and Drop is a very difficult
component to test in automation, however I am currently working on
defining a set of manual tests for the Drag and Drop component, and I'll
make sure that our behavior for "insecure" accesses is tested in those
tests in the future.

MozReview-Commit-ID: 4pnPyL1tgcV
Attachment #8826708 - Flags: review?(amarchesini)
Flags: needinfo?(michael)
Flags: needinfo?(enndeakin)
Assignee: nobody → michael
Attachment #8826708 - Flags: review?(amarchesini) → review?(bkelly)
Comment on attachment 8826708 [details] [diff] [review]
Don't raise NS_ERROR_DOM_SECURITY_ERROR on DataTransfer access violations

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

::: dom/events/DataTransferItem.cpp
@@ +478,5 @@
>    // we only allow access to data of the same principal. During other events,
>    // only allow access to the data with the same principal.
> +  //
> +  // We don't want to fail with an exception in this siutation, rather we want
> +  // to just pretend as though the stored data is "nullptr". This is consisitent

nit: s/consisitent/consistent/g

@@ +480,5 @@
> +  //
> +  // We don't want to fail with an exception in this siutation, rather we want
> +  // to just pretend as though the stored data is "nullptr". This is consisitent
> +  // with Chrome's behavior and is less surprising for web applications which
> +  // don't expect execptions to be raised when performing certain operations.

Web platform test and spec issue filed?
Attachment #8826708 - Flags: review?(bkelly) → review+
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e97457f78bd
Don't raise NS_ERROR_DOM_SECURITY_ERROR on DataTransfer access violations, r=baku
Comment on attachment 8826708 [details] [diff] [review]
Don't raise NS_ERROR_DOM_SECURITY_ERROR on DataTransfer access violations

Approval Request Comment
[Feature/Bug causing the regression]:bug 1296041
[User impact if declined]: drag and drop in onedrive will not work
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: The fix was just landed. It has not been verified in nightly yet but once the next nightly is bundled it can be.
[Needs manual test from QE? If yes, steps to reproduce]: See comment 1
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: Very minor change to behavior of function to move behavior more in line with chrome's Drag and Drop behavior.
[String changes made/needed]: none
Attachment #8826708 - Flags: approval-mozilla-beta?
Attachment #8826708 - Flags: approval-mozilla-aurora?
(In reply to Ben Kelly [:bkelly] from comment #6)
> @@ +480,5 @@
> > +  //
> > +  // We don't want to fail with an exception in this siutation, rather we want
> > +  // to just pretend as though the stored data is "nullptr". This is consisitent
> > +  // with Chrome's behavior and is less surprising for web applications which
> > +  // don't expect execptions to be raised when performing certain operations.
> 
> Web platform test and spec issue filed?

As I mentioned in IRC I think that writing a web platform test for this will be very tricky as I don't believe there are mechanisms for simulating file drags from the operating system (but I would love to be proven wrong). 

I've filed https://github.com/whatwg/html/issues/2267 to clarify the spec.
Thanks.  Can we at least add a mochitest or browser chrome test?
https://hg.mozilla.org/mozilla-central/rev/6e97457f78bd
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8826708 [details] [diff] [review]
Don't raise NS_ERROR_DOM_SECURITY_ERROR on DataTransfer access violations

Fix for release blocking issue with drag and drop, let's take it on aurora and beta.
Michael, thanks very much for the fast response!
Attachment #8826708 - Flags: approval-mozilla-beta?
Attachment #8826708 - Flags: approval-mozilla-beta+
Attachment #8826708 - Flags: approval-mozilla-aurora?
Attachment #8826708 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
Verified fixed on Windows 10 x64, Mac OS X 10.11 and Ubuntu 16.04 x64 using Firefox 51 RC (buildID: 20170116133120).
Verified fixed FX 53.0a1 (2017-01-23), 52.0a2 (2017-01-23) Win 7.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.