RESOLVED FIXED Bug 39029
REGRESSION: (r47291): Upload progress events are not fired for simple cross-origin XHR
https://bugs.webkit.org/show_bug.cgi?id=39029
Summary REGRESSION: (r47291): Upload progress events are not fired for simple cross-o...
David Levin
Reported 2010-05-12 14:56:47 PDT
REGRESSION (r47291): Progress events are no longer fired for simple cross site xhr when the event was set-up before xhr.send is done.
Attachments
Patch (4.89 KB, patch)
2010-05-12 15:02 PDT, David Levin
no flags
Proposed fix v2. (4.64 KB, patch)
2010-05-12 16:43 PDT, David Levin
ap: review+
levin: commit-queue-
David Levin
Comment 1 2010-05-12 15:02:57 PDT
David Levin
Comment 2 2010-05-12 15:16:50 PDT
This is basically a follow up to https://bugs.webkit.org/show_bug.cgi?id=37771 where I looked at the original behavior before r47291 and made the code behave as it did before. I know that this is a step in the right direction. However, I admit to not knowing if the behavior is as complete as before in every circumstance because there are a lot of place that did m_uploadEventsAllowed = true (if you look at http://trac.webkit.org/changeset/47291) which no longer do (a few in makeCrossOriginAccessRequestWithPreflight and one in handleAsynchronousPreflightResult), but I believe that these are handled by the "!isSimpleCrossOriginAccessRequest(m_method, m_requestHeaders)" clause that was already present.
David Levin
Comment 3 2010-05-12 16:43:09 PDT
Created attachment 55911 [details] Proposed fix v2.
Alexey Proskuryakov
Comment 4 2010-05-12 17:01:48 PDT
Comment on attachment 55911 [details] Proposed fix v2. The code changes look good. Maybe I'd have renamed forcePreflight local variable to uploadEvents to match the current spec draft, and to make the code slightly less confusing. According to the spec, the upload events flag should be always set to false for sync requests. So, it's wrong to force preflight because of installed event listeners, and I'm not sure what we do with actual events in this case. + * http/tests/xmlhttprequest/simple-cross-origin-progress-events.html: Add a test which sets up + at least one the progress event listener before the XMLHttpRequest send is done. This only verifies the fix indirectly (by testing that preflight is forced, which is a great thing to test). Can you add a test to verify that progress events for successful upload are fired, too? r=me as is, but please consider investigating and testing the sync case, and adding a test for successful preflight. It may also be interesting to investigate the case where preflight is enabled for some other reason (such as a different content type), and event listeners are only added after send(). WebKit would dispatch upload events, and this seems to be the logical thing to do, but I'm not sure how interpret the spec text.
David Levin
Comment 5 2010-06-22 21:39:57 PDT
David Levin
Comment 6 2010-06-22 21:50:57 PDT
(In reply to comment #4) > (From update of attachment 55911 [details]) > Maybe I'd have renamed forcePreflight local variable to uploadEvents to match the current spec draft. Done. > According to the spec, the upload events flag should be always set to false for sync requests. So, it's wrong to force preflight because of installed event listeners, and I'm not sure what we do with actual events in this case. You were prescient: https://bugs.webkit.org/show_bug.cgi?id=40996 > This only verifies the fix indirectly (by testing that preflight is forced, which is a great thing to test). Can you add a test to verify that progress events for successful upload are fired, too? Filed: https://bugs.webkit.org/show_bug.cgi?id=41038 > > r=me as is, adding a test for successful preflight. Is this the same as bug 41038 or are you expecting something else? (If something else, I'll file another bug for myself.) > It may also be interesting to investigate the case where preflight is enabled for some other reason (such as a different content type), and event listeners are only added after send(). WebKit would dispatch upload events, and this seems to be the logical thing to do, but I'm not sure how interpret the spec text. Filed: https://bugs.webkit.org/show_bug.cgi?id=41041
WebKit Review Bot
Comment 7 2010-06-22 21:59:13 PDT
http://trac.webkit.org/changeset/61654 might have broken Qt Linux Release
David Levin
Comment 8 2010-06-22 22:28:06 PDT
Follow fix to disable the test for Qt: http://trac.webkit.org/changeset/61657
Alexey Proskuryakov
Comment 9 2010-06-23 09:58:05 PDT
> Is this the same as bug 41038 Yes, thanks!
Alexey Proskuryakov
Comment 10 2010-06-23 10:00:37 PDT
Note You need to log in before you can comment on or make changes to this bug.