Bug 37882 - Sending a package file using FormData crashes WebKit on Mac
Summary: Sending a package file using FormData crashes WebKit on Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P1 Normal
Assignee: Jian Li
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-04-20 12:40 PDT by Richard York
Modified: 2010-04-27 14:57 PDT (History)
8 users (show)

See Also:


Attachments
Script Including HTML5 DnD Implementation (2.78 KB, text/html)
2010-04-20 12:40 PDT, Richard York
no flags Details
When I drop this file onto the drop zone, webkit crashes (106.30 KB, application/zip)
2010-04-20 12:42 PDT, Richard York
no flags Details
Proposed Patch (1.92 KB, patch)
2010-04-23 12:14 PDT, Jian Li
adele: review-
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (4.11 KB, patch)
2010-04-27 14:03 PDT, Jian Li
darin: review+
jianli: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Richard York 2010-04-20 12:40:51 PDT
Created attachment 53865 [details]
Script Including HTML5 DnD Implementation

When I drag and drop the attached .pages document onto the drop zone, webkit crashes.
Comment 1 Richard York 2010-04-20 12:42:25 PDT
Created attachment 53866 [details]
When I drop this file onto the drop zone, webkit crashes
Comment 2 Alexey Proskuryakov 2010-04-21 00:58:23 PDT
Confirmed with r57789.

Crashing on a secondary thread in form serialization code (WebCore::advanceCurrentStream()). The test uses XMLHttpRequest to upload FormData.
Comment 3 Jian Li 2010-04-23 12:06:45 PDT
The crash is caused by sending a package file using FormData. Updated the title to reflect this.
Comment 4 Jian Li 2010-04-23 12:14:27 PDT
Created attachment 54180 [details]
Proposed Patch
Comment 5 Alexey Proskuryakov 2010-04-23 12:29:37 PDT
XMLHttpRequest doesn't talk to Page and Chrome, because it can be used from workers. Of course, this version of send() won't be invoked from a worker, since there is no DOMFormData, but it would still be better to not even include these headers in XMLHttpRequest.cpp.

Of course, FormData already does this, and it's used by XMLHttpRequest in all code paths. That's freaky.
Comment 6 Alexey Proskuryakov 2010-04-23 12:35:30 PDT
It's even more freaky because FormData is used from secondary threads on the Mac, even when there are no workers. I can't really follow which methods can be called on which thread, and whether there are race conditions.

It's fine to fix this particular crasher without a huge refactoring. But I'd prefer to not add direct Page/Chrome dependencies to XMLHttpRequest.cpp, if that's possible.
Comment 7 Darin Adler 2010-04-24 09:45:47 PDT
Comment on attachment 54180 [details]
Proposed Patch

This does seem like roughly what we want to do, but I'm concerned about the layering here. Adele Peterson did the original work; she might have some thoughts about the best way to do this. I'll think on it myself too.
Comment 8 Alexey Proskuryakov 2010-04-24 10:57:23 PDT
<rdar://problem/7903467>
Comment 9 Adele Peterson 2010-04-26 11:43:10 PDT
Comment on attachment 54180 [details]
Proposed Patch

Darin suggested we could change FormData::generateFiles to take a Document* instead of a ChromeClient*.  That would make this much cleaner.  Jian, can you give that a try and resubmit?
Comment 10 Maciej Stachowiak 2010-04-27 13:01:02 PDT
Jian, any update on this? It would be valuable to us to have a fix soon. Apple folks can take this one over if you don't have time to revise the patch.
Comment 11 Jian Li 2010-04-27 13:42:04 PDT
Sorry for the delay. I am working on this one now.
Comment 12 Jian Li 2010-04-27 14:03:46 PDT
Created attachment 54450 [details]
Proposed Patch

Thanks for the suggestion. Please see the revised patch.
Comment 13 Jian Li 2010-04-27 14:33:31 PDT
Committed as http://trac.webkit.org/changeset/58336.
Comment 14 WebKit Review Bot 2010-04-27 14:57:15 PDT
http://trac.webkit.org/changeset/58336 might have broken SnowLeopard Intel Release (Tests)