Correction: "Did this work before? Yes Chrome **51** and earlier" (typo, sorry)

thanks for reporting!

This is because:
If we start revalidation and add a ResourceClient during notifyFinished(), the newly added ResourceClient is moved to m_finishedClients in markClientsAndObserversFinished() just after checkNotify(), and thus no further callbacks to the ResourceClient are made.

Bisected on Linux, and found https://codereview.chromium.org/1923003002 was the first bad commit.

I'm preparing a CL.

Regression since M-52.

> https://codereview.chromium.org/1923003002 was the first bad commit
This is because: before the CL, the RawResource for the XHR was removed from MemoryCache in Resource::cancelTimerFired() called from removeClient() called ... in XHR's onload handler, which caused a new Resource to be created and a new load was initiated (not revalidation).
After the CL, RawResource for XHR remains on the MemoryCache and revalidation is initiated on the MemoryCache, which exposes the behavior in Comment 4.

We're noticing the same behavior, so it's definitely there.

Here's a test case in case anyone needs it: https://github.com/mkawalec/chrome-req-bug

We are also experiencing the same in Chrome Version 52.0.2743.116 (64-bit)
 as of yesterday Aug 9, 2016.

Firefox works fine. as did chrome 51.x

Here's an example

Deleted: xhr-304-bug-chrome-52.js 554 bytes

xhr-304-bug-chrome-52.js 554 bytes View Download

The onreadystatechange is not fired in some cases when the server returns 200, at least   when the second request is opened in the onload callback. 

I wrote these tests before finding this issue:
https://github.com/hvrauhal/chrome-ajax-onload-not-fired 

I noticed for us, disabling etag support in express makes the problem go away, so from what I've read today, the 304 is faked in the client side by xhr. For us `app.set('etag', false)` makes the problem go away.

@ettin - yeah, here's the workarounds I can think of:
1. Disable ETags, so this caching mechanism won't be used.
2. Append cache busters to requests – jQuery.ajax() has a `cache` option that can be set to false for this. This is what we've done as a workaround until this is fixed.

 Issue 625656  has been merged into this issue.

Draft CL under review: https://codereview.chromium.org/2210473002/

 Issue 636612  has been merged into this issue.

I was wondering if I could have access to an alpha build with this fix to verify that my issues with Chrome and XHR calls are due to this issue. Or if you can let me know when this fix will be available I would be grateful.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/48136fcd4b053686ce2f1f98e64b00a064907ec4

commit 48136fcd4b053686ce2f1f98e64b00a064907ec4
Author: hiroshige <hiroshige@chromium.org>
Date: Wed Aug 24 05:52:30 2016

Verify the order of RawResourceClient callbacks in DocumentThreadableLoader

The errors in the order of RawResourceClient callbacks potentially leads to
CORS bypassing, especially in DocumentThreadableLoader.
This CL introduces RawResourceClientStateChecker to check the order of
RawResourceClient callbacks, assuming that the RawResourceClient is added
to at most one RawResource, and applies it to DocumentThreadableLoader.

BUG= 633696 , 640291

Review-Url: https://codereview.chromium.org/2020313002
Cr-Commit-Position: refs/heads/master@{#413990}

[modify] https://crrev.com/48136fcd4b053686ce2f1f98e64b00a064907ec4/third_party/WebKit/Source/core/fetch/RawResource.cpp
[modify] https://crrev.com/48136fcd4b053686ce2f1f98e64b00a064907ec4/third_party/WebKit/Source/core/fetch/RawResource.h
[modify] https://crrev.com/48136fcd4b053686ce2f1f98e64b00a064907ec4/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
[modify] https://crrev.com/48136fcd4b053686ce2f1f98e64b00a064907ec4/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h

Sorry for delay.
The code review is done and I'm planning to land a fix this Thursday or Friday on canary.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/62bb97b2652c495598942879e66bd7f1a5dbb918

commit 62bb97b2652c495598942879e66bd7f1a5dbb918
Author: hiroshige <hiroshige@chromium.org>
Date: Thu Aug 25 13:56:31 2016

Mark ResourceClient/ImageResourceObserver finished just before notifying

Previously, all clients (except for awaiting ones) and observers are marked
finished just after checkNotify() is called.
However, it is possible that we have clients in |m_client| newly added during
ResourceClient::notifyFinished(), and such new clients shouldn't be marked
finished without calling notifyFinished().

This CL marks a client/observer just before notifyFinished()/
imageNotifyFinished() is called for the client/observer, and removes
markClientsAndObserversFinished() methods.

This CL also introduces MarkFinishedOption for checkNotify() for multipart
images: In multipart images we call notifyFinished()/imageNotifyFinished()
without marking clients/observers finished when the first part ends.

BUG= 633696 

Review-Url: https://codereview.chromium.org/2210473002
Cr-Commit-Position: refs/heads/master@{#414423}

[add] https://crrev.com/62bb97b2652c495598942879e66bd7f1a5dbb918/third_party/WebKit/LayoutTests/http/tests/cache/resources/etag-200.php
[add] https://crrev.com/62bb97b2652c495598942879e66bd7f1a5dbb918/third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html
[add] https://crrev.com/62bb97b2652c495598942879e66bd7f1a5dbb918/third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-successful-revalidation.html
[modify] https://crrev.com/62bb97b2652c495598942879e66bd7f1a5dbb918/third_party/WebKit/Source/core/fetch/ImageResource.cpp
[modify] https://crrev.com/62bb97b2652c495598942879e66bd7f1a5dbb918/third_party/WebKit/Source/core/fetch/ImageResource.h
[modify] https://crrev.com/62bb97b2652c495598942879e66bd7f1a5dbb918/third_party/WebKit/Source/core/fetch/Resource.cpp
[modify] https://crrev.com/62bb97b2652c495598942879e66bd7f1a5dbb918/third_party/WebKit/Source/core/fetch/Resource.h

To #18. Isn't it a regression which breaks the web? I thought such things should be merged into Stable too.

> #20
Agree, I'll request merging the fix into stable.

Because the fix is not so small/trivial, we have to be more careful when merging the fix to beta/stable.
https://codereview.chromium.org/2020313002 was landed (Comment #17) before the fix (Comment #19) to verify the fix is correct.

I am also seeing that even xhr.onload is not fired for 304 Not Modified requests. Does the commit fixes this too?

When can we expect stable to have the fix?  Echoing https://bugs.chromium.org/p/chromium/issues/detail?id=633696#c20 this regression does break the web.

Since August 8th my company has had 35 customers report bugs, through our support channel, that is related to this regression.  

Hi All,

I see similar problem not only for 304 response, but also for 404 response in Chrome 52. No callback for second xhr request to same url that returns 404. Probable it is related to races in Chrome CORS functionality. In case of break in debugger for few seconds after xhr request performed callback is called.

Requesting merging the fix (CL in Comment #19) to M-53.
The fix is baked on canary/dev, and I confirmed the test case of Comment #0 works fine on 55.0.2847.0 canary.

The CL in Comment #21 caused Issue 640960 (protected) and the issue is now resolved, so I think the fix is safe to merge.

Increasing the priority to Pri-1 because there are multiple reports on this regression.

[Automated comment] Less than 2 weeks to go before stable on M53, manual review required.

> Comment #22
> I am also seeing that even xhr.onload is not fired for 304 Not Modified requests. Does the commit fixes this too?
I expect so.

> Comment #24
Does this occur on the current canary or dev?
If so, could you provide a test case?

I expect this issue occurs when an XHR initiates cache revalidation in onload/onreadystatechange(readyState===4) event of another previous XHR to the same URL, no matter what status code is returned to the second XHR.
So the case in Comment #24 might be the same issue (and thus fixed by the fix in #19).

Approving merge (CL in Comment #19) to M53 branch 2785 based on comment #25 and after chatting with  hiroshige@ as it is baked in Canary over a week. Please merge ASAP so we can pick it up for this week M53 Stable Candidate cut.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f72812fd4fc8d47803ecb6921e4e78c762ecf56f

commit f72812fd4fc8d47803ecb6921e4e78c762ecf56f
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Mon Sep 05 06:59:30 2016

Mark ResourceClient/ImageResourceObserver finished just before notifying

Previously, all clients (except for awaiting ones) and observers are marked
finished just after checkNotify() is called.
However, it is possible that we have clients in |m_client| newly added during
ResourceClient::notifyFinished(), and such new clients shouldn't be marked
finished without calling notifyFinished().

This CL marks a client/observer just before notifyFinished()/
imageNotifyFinished() is called for the client/observer, and removes
markClientsAndObserversFinished() methods.

This CL also introduces MarkFinishedOption for checkNotify() for multipart
images: In multipart images we call notifyFinished()/imageNotifyFinished()
without marking clients/observers finished when the first part ends.

BUG= 633696 

Review-Url: https://codereview.chromium.org/2210473002
Cr-Commit-Position: refs/heads/master@{#414423}
(cherry picked from commit 62bb97b2652c495598942879e66bd7f1a5dbb918)

Review URL: https://codereview.chromium.org/2308393002 .

Cr-Commit-Position: refs/branch-heads/2785@{#823}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[add] https://crrev.com/f72812fd4fc8d47803ecb6921e4e78c762ecf56f/third_party/WebKit/LayoutTests/http/tests/cache/resources/etag-200.php
[add] https://crrev.com/f72812fd4fc8d47803ecb6921e4e78c762ecf56f/third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html
[add] https://crrev.com/f72812fd4fc8d47803ecb6921e4e78c762ecf56f/third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-successful-revalidation.html
[modify] https://crrev.com/f72812fd4fc8d47803ecb6921e4e78c762ecf56f/third_party/WebKit/Source/core/fetch/ImageResource.cpp
[modify] https://crrev.com/f72812fd4fc8d47803ecb6921e4e78c762ecf56f/third_party/WebKit/Source/core/fetch/ImageResource.h
[modify] https://crrev.com/f72812fd4fc8d47803ecb6921e4e78c762ecf56f/third_party/WebKit/Source/core/fetch/Resource.cpp
[modify] https://crrev.com/f72812fd4fc8d47803ecb6921e4e78c762ecf56f/third_party/WebKit/Source/core/fetch/Resource.h

> Comment #23
The fix was merged into stable (Comment #29). I expect it will be available on stable soon.

@hiroshige: Performed the steps mentioned in the bug "625656" which got merged into this bug and observed the behavior [attached screen-recording - 633696 - 53.0.2785.101]

Apparently, observed the same behavior on the Chrome Version [attached screen-recording633696 - 52.0.2743.60]. 

Could you please have a look and confirm if the procedure followed to repro this issue is correct or not.

Tested it on Windows 7 & MAC (10.11.6)

Thank you.

Deleted: 633696 - 53.0.2785.101.mov 16.0 MB

633696 - 53.0.2785.101.mov 16.0 MB Download

Deleted: 633696 - 52.0.2743.60.mov 2.0 MB

633696 - 52.0.2743.60.mov 2.0 MB Download

> Comment #31
Hmm, printing "Done" looks like the expected behavior according to Comment #0 of  Issue 625656 .
Multiple comments in  Issue 625656  also say that they couldn't reproduce on M-52.
I also tested on 55.0.2847.0 and 52.0.2743.116 on Windows 7, both print "Done".

@hiroshige: Based on your comment #33, can we go ahead and add the TE-Verified Labels.

Thank you.

Probably rnimmagadda@ and I just couldn't reproduce the  Issue 625656 .

Can anyone who could reproduce  Issue 625656  check whether it is fixed?

The fix is included in:
53.0.2785.101 or later
54.0.2840.0 or later
55.0.*

Not included in:
53.0.2785.89

@hiroshige:  Our test cases (and affected apps) are now fixed.  This affected a large number of our enterprise applications.  We had numerous customer calls (100+) on bugs related to this regression.  Thanks for getting the patch out as quickly as possible.

A correção foi apresentada com erros 

Yeah thanks @hiroshige - our application works fine after re-enabling caching in the latest Chrome stable.