Closed Bug 1336091 Opened 3 years ago Closed 3 years ago

File.relativeWebkitPath should not start with '/'

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox52 --- verified
firefox-esr52 --- fixed
firefox53 --- verified
firefox54 --- verified

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
Attached patch domPath.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8832860 - Flags: review?(bugs)
Hmm, how did we not catch this earlier
Er, hmm, in which case does this happen? drag-and-drop to <input type=file webkitdirectory> doesn't create / prefixed paths.
Do you have a testcase which works also in Chrome?
(it tends to be hard to write testcase for Chrome, since Entries API is just so broken there, at least on Linux)
Flags: needinfo?(amarchesini)
Attached file report.zip (obsolete) —
Open index.html and d&d dir. On Chrome, webkitRelativePath is "". We return "/dir/test.txt", with my patch: "dir/test.txt".

Interesting thing, with this patch onedrive works, so probably there is an easier way to find the webkitRelativePath.
Flags: needinfo?(amarchesini)
Duplicate of this bug: 1331637
wondering why that testcase has 'directory' attribute
And the testcase doesn't work on Chrome, at least not on Version 57.0.2986.0
Comment on attachment 8832860 [details] [diff] [review]
domPath.patch

apparently a new patch is coming.
Attachment #8832860 - Flags: review?(bugs)
Attached patch domPath.patchSplinter Review
mochitest included
Attachment #8832860 - Attachment is obsolete: true
Attachment #8833229 - Attachment is obsolete: true
Attachment #8833335 - Flags: review?(bugs)
So a (manual) testcase which works in Chrome would be nice.
So with the patch path is the absolute path in file system, dom path is the web exposed path with / prefix and relative path is the web exposed path without / prefix?
And without the patch path is usually same as relative path, but in some case dom path?
Comment on attachment 8833335 [details] [diff] [review]
domPath.patch

Could you explain why webkitRelativePath does normally work ok, but only with dnd it breaks?
If you access input.files, those have the right path already.


> FileSystemFileEntry::GetFile(FileCallback& aSuccessCallback,
>                              const Optional<OwningNonNull<ErrorCallback>>& aErrorCallback) const
> {
>   RefPtr<FileCallbackRunnable> runnable =
>-    new FileCallbackRunnable(&aSuccessCallback, mFile);
>+    new FileCallbackRunnable(&aSuccessCallback,
>+                             aErrorCallback.WasPassed()
>+                               ? &aErrorCallback.Value() : nullptr,
>+                             mFile);
ok, some unrelated changes too, like error callback handling.

 
> function test_basic(aDirectory, aNext) {
>   ok(aDirectory, "Directory exists.");
>         if (data[i] instanceof File) {
>-          is(data[i].webkitRelativePath, createPath(aDirectory, data[i]), "File.webkitRelativePath should be called '/' + file.name: " + data[i].webkitRelativePath);
>+          is(data[i].webkitRelativePath, createRelativePath(aDirectory, data[i]), "File.webkitRelativePath should be called '/' + file.name: " + data[i].webkitRelativePath);
The comment looks wrong, since webkitRelativePath should be /file.name  but just file.name
Attachment #8833335 - Flags: review?(bugs) → review+
> Could you explain why webkitRelativePath does normally work ok, but only
> with dnd it breaks?

When webkitdirectory is processed, we use GetFilesHelper. This object takes the leafName of the directory and it uses as base dir name. Each sub file/dir will have a path generated as base_dir + '/' + subfile.leafName.

Differently, when we do input.webkitEntries... we use GetDirectoryListingTask. Here we have a path, taken from the directory, and this starts with '/'. Any generated string will be '/' + dir_name + '/' + subfile.leafName.
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef7edd52783
File.relativeWebkitPath should not start with '/', r=smaug
https://hg.mozilla.org/mozilla-central/rev/4ef7edd52783
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
I just tested to make sure that bug 1331637 is resolved by this, and it appears to be!

How risky would this be to uplift? as I believe that onedrive is broken in 51+ for uploading folders right now.
Flags: needinfo?(amarchesini)
I was just checking STR from bug 1331637. On latest Nightly 54.0a1 2017-02-07 I can indeed upload files, as :mystor already mentioned, but the below errors appear on web console at least as long as the upload process continues.

XML Parsing Error: no root element found Location: https://onedrive.live.com/log Line Number 1, Column 1:  log:1:1
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:  

Are there any other test cases we could test with? I saw that the initial test case added here is obsolete now.
Comment on attachment 8833335 [details] [diff] [review]
domPath.patch

Approval Request Comment
[Feature/Bug causing the regression]: Entries API and Drag&Drop
[User impact if declined]: onedrive.com doesn't work
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: also. Test the uploading of files to onedrive.com
[List of other uplifts needed for the feature/fix]: none. Probably a rebase is needed
[Is the change risky?]: none
[Why is the change risky/not risky?]:The patch is big because there are some renaming.
[String changes made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8833335 - Flags: approval-mozilla-beta?
Attachment #8833335 - Flags: approval-mozilla-aurora?
Hi :baku,
According to comment #19, are the errors as expected?
Flags: needinfo?(amarchesini)
> According to comment #19, are the errors as expected?

I don't think they are related. There are no XML involved in the Entries API.
Flags: needinfo?(amarchesini)
Attachment #8833335 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8833335 [details] [diff] [review]
domPath.patch

Fix the uploading files issue to onedrive.com. Aurora53+.
Comment on attachment 8833335 [details] [diff] [review]
domPath.patch

webkitRelativePath fix, beta52+
Attachment #8833335 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This needs a rebased patch for Beta uplift.
Flags: needinfo?(amarchesini)
Attached patch m-bSplinter Review
Flags: needinfo?(amarchesini)
Verified as fixed using steps from bug 1331637 on Firefox 52 beta 7 and latest Aurora 53.0a2 under Win 10 64-bit, Ubuntu 14.10 64-bit and Mac OS X 10.12.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.