91630 – Web Inspector: Incorrect XHR responses when two async xhrs are sent synchronously

RESOLVED FIXED 91630

Web Inspector: Incorrect XHR responses when two async xhrs are sent synchronously

https://bugs.webkit.org/show_bug.cgi?id=91630

Summary Web Inspector: Incorrect XHR responses when two async xhrs are sent synchrono...

Pavel Chadnov

Reported 2012-07-18 08:38:52 PDT

Incorrect XHR responses in Network tab when sending two XHRs with small delay.

Attachments
Patch (6.40 KB, patch) 2012-07-19 02:55 PDT, Pavel Chadnov	no flags	Details Formatted Diff Diff
Patch (9.86 KB, patch) 2012-07-19 04:45 PDT, Pavel Chadnov	no flags	Details Formatted Diff Diff
Patch (14.87 KB, patch) 2012-07-20 09:26 PDT, Pavel Chadnov	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-06 (307.79 KB, application/zip) 2012-07-20 10:23 PDT, WebKit Review Bot	no flags	Details
Patch (16.20 KB, patch) 2012-08-07 06:04 PDT, Pavel Chadnov	no flags	Details Formatted Diff Diff
Patch (16.10 KB, patch) 2012-08-07 06:10 PDT, Pavel Chadnov	no flags	Details Formatted Diff Diff
Patch (21.51 KB, patch) 2012-08-07 06:39 PDT, Pavel Chadnov	no flags	Details Formatted Diff Diff
Patch (21.49 KB, patch) 2012-08-07 07:36 PDT, Pavel Chadnov	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-02 (429.83 KB, application/zip) 2012-08-07 08:39 PDT, WebKit Review Bot	no flags	Details
Patch (25.47 KB, patch) 2012-08-14 05:53 PDT, Pavel Chadnov	no flags	Details Formatted Diff Diff
Patch (24.03 KB, patch) 2012-08-14 06:28 PDT, Pavel Chadnov	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-02 (559.64 KB, application/zip) 2012-08-14 07:47 PDT, WebKit Review Bot	no flags	Details
Patch (25.80 KB, patch) 2012-08-15 03:01 PDT, Pavel Chadnov	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-01 (569.95 KB, application/zip) 2012-08-15 03:58 PDT, WebKit Review Bot	no flags	Details
Patch (29.55 KB, patch) 2012-08-15 05:41 PDT, Pavel Chadnov	no flags	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Vsevolod Vlasov

Comment 1 2012-07-18 09:56:12 PDT

Chromium bug report: http://code.google.com/p/chromium/issues/detail?id=133021

Pavel Chadnov

Comment 2 2012-07-19 02:55:13 PDT

Created attachment 153215 [details] Patch

Build Bot

Comment 3 2012-07-19 03:03:51 PDT

Comment on attachment 153215 [details] Patch Attachment 153215 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13282856

Vsevolod Vlasov

Comment 4 2012-07-19 03:07:08 PDT

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.

Pavel Chadnov

Comment 5 2012-07-19 04:45:26 PDT

Created attachment 153229 [details] Patch

Build Bot

Comment 6 2012-07-19 04:54:27 PDT

Comment on attachment 153229 [details] Patch Attachment 153229 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13276880

Pavel Feldman

Comment 7 2012-07-20 01:55:12 PDT

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

Pavel Chadnov

Comment 8 2012-07-20 09:26:05 PDT

Created attachment 153523 [details] Patch

WebKit Review Bot

Comment 9 2012-07-20 09:29:10 PDT

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.

WebKit Review Bot

Comment 10 2012-07-20 10:23:12 PDT

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

WebKit Review Bot

Comment 11 2012-07-20 10:23:17 PDT

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

Vsevolod Vlasov

Comment 12 2012-07-24 08:24:54 PDT

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.

Pavel Chadnov

Comment 13 2012-08-07 06:04:48 PDT

Created attachment 156924 [details] Patch

Vsevolod Vlasov

Comment 14 2012-08-07 06:06:56 PDT

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.

Pavel Chadnov

Comment 15 2012-08-07 06:10:39 PDT

Created attachment 156926 [details] Patch

Vsevolod Vlasov

Comment 16 2012-08-07 06:22:32 PDT

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

Pavel Chadnov

Comment 17 2012-08-07 06:39:13 PDT

Created attachment 156929 [details] Patch

Vsevolod Vlasov

Comment 18 2012-08-07 06:50:48 PDT

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.

Pavel Chadnov

Comment 19 2012-08-07 07:36:21 PDT

Created attachment 156938 [details] Patch

WebKit Review Bot

Comment 20 2012-08-07 08:39:06 PDT

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

WebKit Review Bot

Comment 21 2012-08-07 08:39:11 PDT

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

Vsevolod Vlasov

Comment 22 2012-08-07 23:02:15 PDT

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.

Pavel Chadnov

Comment 23 2012-08-14 05:53:27 PDT

Created attachment 158306 [details] Patch

Vsevolod Vlasov

Comment 24 2012-08-14 06:15:01 PDT

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.

Pavel Chadnov

Comment 25 2012-08-14 06:28:20 PDT

Created attachment 158313 [details] Patch

Gyuyoung Kim

Comment 26 2012-08-14 06:51:06 PDT

Comment on attachment 158313 [details] Patch Attachment 158313 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13502193

Build Bot

Comment 27 2012-08-14 07:22:47 PDT

Comment on attachment 158313 [details] Patch Attachment 158313 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13493624

Build Bot

Comment 28 2012-08-14 07:30:06 PDT

Comment on attachment 158313 [details] Patch Attachment 158313 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13502208

Early Warning System Bot

Comment 29 2012-08-14 07:34:58 PDT

Comment on attachment 158313 [details] Patch Attachment 158313 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13500267

WebKit Review Bot

Comment 30 2012-08-14 07:47:54 PDT

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

WebKit Review Bot

Comment 31 2012-08-14 07:47:59 PDT

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

Vsevolod Vlasov

Comment 32 2012-08-14 07:51:10 PDT

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.

Early Warning System Bot

Comment 33 2012-08-14 08:18:09 PDT

Comment on attachment 158313 [details] Patch Attachment 158313 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13485933

Vsevolod Vlasov

Comment 34 2012-08-15 00:05:10 PDT

Comment on attachment 158313 [details] Patch Please extract 304 check and fix compilation problems.

Pavel Chadnov

Comment 35 2012-08-15 03:01:27 PDT

Created attachment 158535 [details] Patch

WebKit Review Bot

Comment 36 2012-08-15 03:58:25 PDT

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

WebKit Review Bot

Comment 37 2012-08-15 03:58:31 PDT

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

Pavel Chadnov

Comment 38 2012-08-15 05:41:04 PDT

Created attachment 158551 [details] Patch

Vsevolod Vlasov

Comment 39 2012-08-15 09:50:32 PDT

Comment on attachment 158551 [details] Patch Clearing flags on attachment: 158551 Committed r125681: <http://trac.webkit.org/changeset/125681>

Vsevolod Vlasov

Comment 40 2012-08-15 09:50:43 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component Web Inspector (Deprecated)

Assignee

Nobody

Reported

2012-07-18 08:38 PDT

Modified

2012-08-15 09:50 PDT History

CC List

18 users Show

URL

http://code.google.com/p/chromium/issues/detail?id=133021

Keywords

Depends on

Blocks