WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Proposed fix v2.
(4.64 KB, patch)
2010-05-12 16:43 PDT
,
David Levin
ap
: review+
levin
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
David Levin
Comment 1
2010-05-12 15:02:57 PDT
Created
attachment 55906
[details]
Patch
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
Committed as
http://trac.webkit.org/changeset/61654
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
<
rdar://problem/8122461
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug