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, gns, japhet, jochen, joepeck, keishi, loislo, pfeldman, philn, pmuellr, rik, timothy, vsevik, webkit.review.bot, xan.lopez, yurys
Priority: P2    
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

Description Pavel Chadnov 2012-07-18 08:38:52 PDT
Incorrect XHR responses in Network tab when sending two XHRs with small delay.
Comment 1 Vsevolod Vlasov 2012-07-18 09:56:12 PDT
Chromium bug report: http://code.google.com/p/chromium/issues/detail?id=133021
Comment 2 Pavel Chadnov 2012-07-19 02:55:13 PDT
Created attachment 153215 [details]
Patch
Comment 3 Build Bot 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
Comment 4 Vsevolod Vlasov 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.
Comment 5 Pavel Chadnov 2012-07-19 04:45:26 PDT
Created attachment 153229 [details]
Patch
Comment 6 Build Bot 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
Comment 7 Pavel Feldman 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
Comment 8 Pavel Chadnov 2012-07-20 09:26:05 PDT
Created attachment 153523 [details]
Patch
Comment 9 WebKit Review Bot 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.
Comment 10 WebKit Review Bot 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
Comment 11 WebKit Review Bot 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
Comment 12 Vsevolod Vlasov 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.
Comment 13 Pavel Chadnov 2012-08-07 06:04:48 PDT
Created attachment 156924 [details]
Patch
Comment 14 Vsevolod Vlasov 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.
Comment 15 Pavel Chadnov 2012-08-07 06:10:39 PDT
Created attachment 156926 [details]
Patch
Comment 16 Vsevolod Vlasov 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
Comment 17 Pavel Chadnov 2012-08-07 06:39:13 PDT
Created attachment 156929 [details]
Patch
Comment 18 Vsevolod Vlasov 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.
Comment 19 Pavel Chadnov 2012-08-07 07:36:21 PDT
Created attachment 156938 [details]
Patch
Comment 20 WebKit Review Bot 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
Comment 21 WebKit Review Bot 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
Comment 22 Vsevolod Vlasov 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.
Comment 23 Pavel Chadnov 2012-08-14 05:53:27 PDT
Created attachment 158306 [details]
Patch
Comment 24 Vsevolod Vlasov 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.
Comment 25 Pavel Chadnov 2012-08-14 06:28:20 PDT
Created attachment 158313 [details]
Patch
Comment 26 Gyuyoung Kim 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
Comment 27 Build Bot 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
Comment 28 Build Bot 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
Comment 29 Early Warning System Bot 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
Comment 30 WebKit Review Bot 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
Comment 31 WebKit Review Bot 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
Comment 32 Vsevolod Vlasov 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.
Comment 33 Early Warning System Bot 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
Comment 34 Vsevolod Vlasov 2012-08-15 00:05:10 PDT
Comment on attachment 158313 [details]
Patch

Please extract 304 check and fix compilation problems.
Comment 35 Pavel Chadnov 2012-08-15 03:01:27 PDT
Created attachment 158535 [details]
Patch
Comment 36 WebKit Review Bot 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
Comment 37 WebKit Review Bot 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
Comment 38 Pavel Chadnov 2012-08-15 05:41:04 PDT
Created attachment 158551 [details]
Patch
Comment 39 Vsevolod Vlasov 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>
Comment 40 Vsevolod Vlasov 2012-08-15 09:50:43 PDT
All reviewed patches have been landed.  Closing bug.