[Bug 38711] DragData::asURL() shouldn't do file validity checks

0 views
Skip to first unread message

bugzill...@webkit.org

unread,
May 12, 2010, 1:40:06 PM5/12/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38711


Daniel Cheng <dch...@chromium.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #55633|0 |1
is obsolete| |
Attachment #55633|review? |
Flag| |
Attachment #55867| |review?, commit-queue?
Flag| |




--- Comment #8 from Daniel Cheng <dch...@chromium.org> 2010-05-12 10:40:03 PST ---
Created an attachment (id=55867)
--> (https://bugs.webkit.org/attachment.cgi?id=55867)
Patch

--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

bugzill...@webkit.org

unread,
May 12, 2010, 2:36:07 PM5/12/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38711


Daniel Cheng <dch...@chromium.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |cb...@apple.com,
| |oli...@apple.com




--- Comment #9 from Daniel Cheng <dch...@chromium.org> 2010-05-12 11:36:03 PST ---
I just noticed that the original issue the code was trying to work around was to prevent Safari from handling any directory loads. However, other WebKit clients can handle this request correctly, so I think the correct place to move this logic is into the Safari loaders. I'm going to poke around the loader code and see where the best place to add this is; I'm not really familiar with it though, so any tips would be welcome...

bugzill...@webkit.org

unread,
May 12, 2010, 2:37:14 PM5/12/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38711


Daniel Cheng <dch...@chromium.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #55867|review?, commit-queue? |review-, commit-queue-
Flag| |




--- Comment #10 from Daniel Cheng <dch...@chromium.org> 2010-05-12 11:37:09 PST ---
(From update of attachment 55867)
Any comments on the existing patch would be welcome as well, while I work on the loader change.

bugzill...@webkit.org

unread,
May 12, 2010, 5:32:53 PM5/12/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38711


Daniel Cheng <dch...@chromium.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #55867|0 |1
is obsolete| |
Attachment #55903| |review?, commit-queue?
Flag| |




--- Comment #11 from Daniel Cheng <dch...@chromium.org> 2010-05-12 14:32:49 PST ---
Created an attachment (id=55903)
--> (https://bugs.webkit.org/attachment.cgi?id=55903)
Patch

bugzill...@webkit.org

unread,
May 13, 2010, 3:14:29 AM5/13/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38711


Evan Martin <ev...@chromium.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #55903|commit-queue? |commit-queue-
Flag| |




--- Comment #12 from Evan Martin <ev...@chromium.org> 2010-05-13 00:14:26 PST ---
(From update of attachment 55903)
Setting commit-queue minus just so this doesn't get accidentally committed until you've verified it won't break Safari.

bugzill...@webkit.org

unread,
May 13, 2010, 1:58:19 PM5/13/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38711


Jian Li <jia...@chromium.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #55903|review? |review-
Flag| |




--- Comment #13 from Jian Li <jia...@chromium.org> 2010-05-13 10:58:16 PST ---
(From update of attachment 55903)
Please also ask someone who is familiar with WebNSPasteboardExtras.mm to take a look.

LayoutTests/ChangeLog:3
+ DragData::asURL() shouldn't do file validity checks
Since you also touch WebNSPasteboardExtras.mm, could you please update the bug title and all ChangeLog to describe the problem in generic way?

LayoutTests/editing/pasteboard/file-input-files-access.html: 
+ <script src="../../fast/js/resources/js-test-post.js"></script>
Why do we need to remove this?

LayoutTests/editing/pasteboard/script-tests/file-input-files-access.js:112
+ // It'd be easiest to use try-finally, but it doesn't work.
This comment is somewhat confusing. Can you explain?

LayoutTests/editing/pasteboard/script-tests/file-input-files-access.js:115
+ // file input here.
Don't need to fold into 2 lines.

WebKit/mac/Misc/WebNSPasteboardExtras.mm:150
+ isDirectory)
Don't need to fold into 2 lines.

WebKit/mac/ChangeLog:11
+ handling, since the Mac loader doesn't work correctly with them.
I think there is a risk condition here. If the directory does not exist at first, [NSPasteboard _web_bestURL] will return a file URL. Then the directory is created.

bugzill...@webkit.org

unread,
May 16, 2010, 12:00:47 AM5/16/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38711


Daniel Cheng <dch...@chromium.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #55903|0 |1
is obsolete| |
Attachment #55903|review-, commit-queue- |
Flag| |

bugzill...@webkit.org

unread,
May 16, 2010, 12:00:56 AM5/16/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38711


Daniel Cheng <dch...@chromium.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #56174| |review?
Flag| |




--- Comment #14 from Daniel Cheng <dch...@chromium.org> 2010-05-15 21:00:53 PST ---
Created an attachment (id=56174)
--> (https://bugs.webkit.org/attachment.cgi?id=56174)
Patch

bugzill...@webkit.org

unread,
May 16, 2010, 12:06:03 AM5/16/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38711





--- Comment #15 from Daniel Cheng <dch...@chromium.org> 2010-05-15 21:06:00 PST ---
(In reply to comment #13)
> (From update of attachment 55903 [details])
> Please also ask someone who is familiar with WebNSPasteboardExtras.mm to take a look.
>

I've asked oli...@apple.com to take a look.

> LayoutTests/ChangeLog:3
> + DragData::asURL() shouldn't do file validity checks
> Since you also touch WebNSPasteboardExtras.mm, could you please update the bug title and all ChangeLog to describe the problem in generic way?
>

I updated the ChangeLogs to try to phrase the issue more clearly.

> LayoutTests/editing/pasteboard/file-input-files-access.html: 
> + <script src="../../fast/js/resources/js-test-post.js"></script>
> Why do we need to remove this?
>

This is removed. Since the final drop test triggers a navigation, nothing executes after that. That's why the usual post-test script is manually embedded in onbeforeunload.

> LayoutTests/editing/pasteboard/script-tests/file-input-files-access.js:112
> + // It'd be easiest to use try-finally, but it doesn't work.
> This comment is somewhat confusing. Can you explain?
>

I removed the comment, but originally, I was hoping that I could do:
try {
// something that triggers navigation
} finally {
// verify test results
}

But finally blocks do not execute if the UA is navigating away from the page.

> LayoutTests/editing/pasteboard/script-tests/file-input-files-access.js:115
> + // file input here.
> Don't need to fold into 2 lines.
>

Fixed.

> WebKit/mac/Misc/WebNSPasteboardExtras.mm:150
> + isDirectory)
> Don't need to fold into 2 lines.
>

Fixed.

> WebKit/mac/ChangeLog:11
> + handling, since the Mac loader doesn't work correctly with them.
> I think there is a risk condition here. If the directory does not exist at first, [NSPasteboard _web_bestURL] will return a file URL. Then the directory is created.

The original code has race conditions too. Maybe the file exists but is deleted before the loader processes it, or maybe something is initially a file and then gets deleted and a directory with the same name is created. There's no real easy way to solve this without changing the loader, and I am not comfortable changing the Mac loader.

bugzill...@webkit.org

unread,
May 16, 2010, 3:35:19 AM5/16/10
to webkit-bo...@googlegroups.com
https://bugs.webkit.org/show_bug.cgi?id=38711


Eric Seidel <er...@webkit.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #55328|review+, commit-queue- |
Flag| |




--- Comment #16 from Eric Seidel <er...@webkit.org> 2010-05-16 00:35:14 PST ---
(From update of attachment 55328)
Cleared Jian Li's review+ from obsolete attachment 55328 so that this bug does not appear in http://webkit.org/pending-commit.
Reply all
Reply to author
Forward
0 new messages