Bug 39029 – REGRESSION: (r47291): Upload progress events are not fired for simple cross-origin XHR

• Component: New Bugs
• Status: RESOLVED FIXED

[David Levin]

REGRESSION (r47291): Progress events are no longer fired for simple cross site xhr when the event was set-up before xhr.send is done.

[David Levin]

Created attachment 55906 [details]
Patch

[David Levin]

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]

Created attachment 55911 [details]
Proposed fix v2.

[Alexey Proskuryakov]

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]

Committed as http://trac.webkit.org/changeset/61654

[David Levin]

(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]

http://trac.webkit.org/changeset/61654 might have broken Qt Linux Release

[David Levin]

Follow fix to disable the test for Qt: http://trac.webkit.org/changeset/61657

[Alexey Proskuryakov]

> Is this the same as bug 41038

Yes, thanks!

[Alexey Proskuryakov]

<rdar://problem/8122461>