Bug 91630

Summary:

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

Product:

WebKit

Reporter:

Pavel Chadnov <chadnov>

Component:

Web Inspector (Deprecated)

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

apavlov, bweinstein, dglazkov, gustavo, japhet, jochen, joepeck, keishi, loislo, pfeldman, philn, pmuellr, rik, timothy, vsevik, webkit.review.bot, xan.lopez, yurys

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

URL:

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

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from gce-cr-linux-06	none
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from gce-cr-linux-02	none
Patch	none
Patch	none
Archive of layout-test-results from gce-cr-linux-02	none
Patch	none
Archive of layout-test-results from gce-cr-linux-01	none
Patch	none

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.