Incorrect XHR responses in Network tab when sending two XHRs with small delay.
Chromium bug report: http://code.google.com/p/chromium/issues/detail?id=133021
Created attachment 153215 [details] Patch
Comment on attachment 153215 [details] Patch Attachment 153215 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13282856
Comment on attachment 153215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153215&action=review > LayoutTests/http/tests/inspector/network-test.js:28 > +function doXHR(method, url, async, callback, payload) callback should be the last parameter for consistency. If you don't want to update other tests you can introduce a new doXHRWithPayload and reuse it in doXHR. > LayoutTests/http/tests/inspector/network/network-xhr-async-double.html:19 > + console.log('both loaded'); Could you please replace it with "Done." for consistency with other tests. > LayoutTests/http/tests/inspector/network/network-xhr-async-double.html:60 > + InspectorTest.assertTrue(request1.content=='request1' && request2.content=='request2'); We use === in javascript. Also please add spaces around it. Let's dump xhrs content here as well. > LayoutTests/http/tests/inspector/network/network-xhr-async-double.html:67 > +<body onload="runTest()"></body> Please add a brief description of what you are testing here and a link to the bug. > LayoutTests/http/tests/inspector/network/resources/echo.php:1 > +<?php Please give a name that makes more sense, e.g. echo-payload.php > LayoutTests/http/tests/inspector/network/resources/echo.php:2 > + Please remove this line.
Created attachment 153229 [details] Patch
Comment on attachment 153229 [details] Patch Attachment 153229 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13276880
Comment on attachment 153229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153229&action=review Per our discussion offline, we should try taking a different approach first where ChacherResource is passed into didReceiveResponse explicitly. > Source/WebCore/ChangeLog:10 > + * inspector/InspectorPageAgent.cpp: Please explain
Created attachment 153523 [details] Patch
Attachment 153523 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/inspector/InspectorInstrume..." exit_code: 1 Source/WebCore/loader/ResourceLoadNotifier.h:34: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/loader/ResourceLoadNotifier.h:62: The parameter name "resourceLoader" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/inspector/InspectorInstrumentation.h:171: The parameter name "resourceLoader" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/loader/SubresourceLoader.cpp:124: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/loader/SubresourceLoader.cpp:124: Missing space before { [whitespace/braces] [5] Source/WebCore/loader/SubresourceLoader.cpp:147: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/loader/SubresourceLoader.cpp:147: Missing space before { [whitespace/braces] [5] Source/WebCore/inspector/InspectorResourceAgent.cpp:65: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/loader/ResourceLoader.cpp:217: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/loader/ResourceLoader.cpp:217: Missing space before { [whitespace/braces] [5] Total errors found: 10 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 153523 [details] Patch Attachment 153523 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13315265 New failing tests: http/tests/inspector/network/network-disable-cache-memory.html http/tests/inspector/resource-har-conversion.html http/tests/inspector/network/network-disable-cache-xhrs.html
Created attachment 153531 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 153523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153523&action=review Please fix style nits and add ChangeLogs. > Source/WebCore/inspector/InspectorInstrumentation.cpp:678 > + InspectorInstrumentation::didReceiveResourceResponse(cookie, identifier, loader, r, 0); You don't need 0 here because you resourceLoader parameter has default value. > Source/WebCore/inspector/InspectorResourceAgent.cpp:246 > + CachedResource* cachedResource = (resourceLoader && resourceLoader->isSubresourceLoader()) ? reinterpret_cast<SubresourceLoader*>(resourceLoader)->cachedResource() : InspectorPageAgent::cachedResource(loader->frame(), response.url()); You should use static_cast instead. Also this line is too hard to read, please use *if* instead of ?: Also please always fallback to InspectorPageAgent::cachedResource.
Created attachment 156924 [details] Patch
Comment on attachment 156924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156924&action=review > Source/WebCore/ChangeLog:7 > + Please add a brief description of the change. > Source/WebCore/inspector/InspectorResourceAgent.cpp:-65 > - Please revert this change.
Created attachment 156926 [details] Patch
Comment on attachment 156926 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156926&action=review Test is missing. > Source/WebCore/ChangeLog:6 > + CachedResource object for XHR response is now taken from ResourceLoader (if it's possible). Please reorder description and Reviewed by
Created attachment 156929 [details] Patch
Comment on attachment 156929 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156929&action=review > Source/WebCore/inspector/InspectorResourceAgent.h:38 > +#include "SubresourceLoader.h" Please remove this includes. > Source/WebCore/loader/ResourceLoadNotifier.h:33 > +#include "ResourceLoader.h" Please remove > Source/WebCore/loader/ResourceLoadNotifier.h:-34 > - Please revert.
Created attachment 156938 [details] Patch
Comment on attachment 156938 [details] Patch Attachment 156938 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13448634 New failing tests: http/tests/inspector/network/network-xhr-same-url-as-main-resource.html http/tests/inspector/network/network-disabling-check-no-memory-leak.html http/tests/inspector/network/network-content-replacement-xhr.html http/tests/inspector/console-xhr-logging.html http/tests/inspector/console-xhr-logging-async.html http/tests/inspector/network/async-xhr-json-mime-type.html http/tests/inspector/resource-tree/resource-tree-no-xhrs.html http/tests/inspector/appcache/appcache-swap.html http/tests/inspector/network/network-xhr-sync.html http/tests/inspector/network/network-xhr-async-double.html http/tests/inspector/network/network-disable-cache-memory.html http/tests/inspector/network/network-xhr-async.html http/tests/inspector/network/network-sidebar-width.html http/tests/inspector/network/network-empty-xhr.html http/tests/inspector/resource-har-conversion.html http/tests/inspector/network/network-cyrillic-xhr.html http/tests/inspector/network/network-disable-cache-xhrs.html
Created attachment 156947 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 156938 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156938&action=review Please fix the tests. > Source/WebCore/inspector/InspectorInstrumentation.h:175 > + static void didReceiveResourceResponse(const InspectorInstrumentationCookie&, unsigned long identifier, DocumentLoader*, const ResourceResponse&, ResourceLoader* = 0); nit: I don't think we should have default argument values for this method. > Source/WebCore/loader/ResourceLoadNotifier.cpp:126 > +void ResourceLoadNotifier::dispatchDidReceiveResponse(DocumentLoader* loader, unsigned long identifier, const ResourceResponse& r, ResourceLoader *resourceLoader) ResourceLoader* resourceLoader > LayoutTests/http/tests/inspector/network-test.js:30 > + doXHRWithPayload(method, url, async, payload, null); callback is missing.
Created attachment 158306 [details] Patch
Comment on attachment 158306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158306&action=review > Source/WebCore/loader/SubresourceLoader.cpp:204 > + m_isRevalidated = true; Adding new field seems too intrusive, instrumentation call would be enough. But after looking at it for a bit more I see that we already have a 304 check in InspectorResourceAgent::didReceiveResponse, we could just reuse it.
Created attachment 158313 [details] Patch
Comment on attachment 158313 [details] Patch Attachment 158313 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13502193
Comment on attachment 158313 [details] Patch Attachment 158313 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13493624
Comment on attachment 158313 [details] Patch Attachment 158313 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13502208
Comment on attachment 158313 [details] Patch Attachment 158313 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13500267
Comment on attachment 158313 [details] Patch Attachment 158313 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13489693 New failing tests: http/tests/inspector/console-xhr-logging.html http/tests/inspector/console-xhr-logging-async.html
Created attachment 158328 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 158313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158313&action=review > Source/WebCore/inspector/InspectorResourceAgent.cpp:249 > + if (resourceLoader && resourceLoader->isSubresourceLoader() && response.httpStatusCode() != 304) Please extract isNotModified.
Comment on attachment 158313 [details] Patch Attachment 158313 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13485933
Comment on attachment 158313 [details] Patch Please extract 304 check and fix compilation problems.
Created attachment 158535 [details] Patch
Comment on attachment 158535 [details] Patch Attachment 158535 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13494936 New failing tests: http/tests/inspector/console-xhr-logging.html http/tests/inspector/console-xhr-logging-async.html
Created attachment 158542 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 158551 [details] Patch
Comment on attachment 158551 [details] Patch Clearing flags on attachment: 158551 Committed r125681: <http://trac.webkit.org/changeset/125681>
All reviewed patches have been landed. Closing bug.