⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 293264 in webkit


Ignore:
Timestamp:
Apr 22, 2022, 4:19:24 PM (4 years ago)
Author:
Patrick Angle
Message:

Web Inspector: Regression(r287684) Resources from the memory cache show empty content in Network, Sources, and Search tabs
​https://bugs.webkit.org/show_bug.cgi?id=239667

Reviewed by Devin Rousso.

Source/WebCore:

Updated tests:

  • http/tests/inspector/network/resource-response-source-disk-cache.html
  • http/tests/inspector/network/resource-response-source-memory-cache.html

r287684 introduced a subtle bug when calling InspectorInstrumentation::didReceiveData. We rely
on there being a difference between real, but empty, data buffer and not having a data buffer,
but after r287684 an empty SharedBuffer would be passed in the later case. Instead, we should
continue to pass nullptr if there is no buffer so that InspectorNetworkAgent::didReceiveData
can distiguish between the two.

The bug is the result of having a non-nullptr data in InspectorNetworkAgent::didReceiveData,
which causes us to call maybeAddResourceData, which means by the time
InspectorNetworkAgent::getResponseBody is called, the ResourceData for the request will have
an empty, not non-existant, content(), which means we will return the empty content instead,
since we believe the response had actual content.

  • inspector/InspectorInstrumentation.cpp:

(WebCore::InspectorInstrumentation::didReceiveDataImpl):

  • inspector/InspectorInstrumentation.h:

(WebCore::InspectorInstrumentation::didReceiveData):

  • loader/ResourceLoadNotifier.cpp:

(WebCore::ResourceLoadNotifier::dispatchDidReceiveData):

LayoutTests:

Add test steps to ensure that the resource has content, and that the base64Encoded value matches our
expectations. While only the memory cache was affected by this regression, add the test steps to both
the memory and disk caches to defend this functionality.

  • http/tests/inspector/network/resource-response-source-memory-cache-expected.txt:
  • http/tests/inspector/network/resource-response-source-memory-cache.html:
  • http/tests/inspector/network/resource-response-source-disk-cache-expected.txt:
  • http/tests/inspector/network/resource-response-source-disk-cache.html:
Location:
trunk
Files:
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r293260 r293264  
     12022-04-22  Patrick Angle  <pangle@apple.com>
     2
     3        Web Inspector: Regression(r287684) Resources from the memory cache show empty content in Network, Sources, and Search tabs
     4        https://bugs.webkit.org/show_bug.cgi?id=239667
     5
     6        Reviewed by Devin Rousso.
     7
     8        Add test steps to ensure that the resource has content, and that the base64Encoded value matches our
     9        expectations. While only the memory cache was affected by this regression, add the test steps to both
     10        the memory and disk caches to defend this functionality.
     11
     12        * http/tests/inspector/network/resource-response-source-memory-cache-expected.txt:
     13        * http/tests/inspector/network/resource-response-source-memory-cache.html:
     14        * http/tests/inspector/network/resource-response-source-disk-cache-expected.txt:
     15        * http/tests/inspector/network/resource-response-source-disk-cache.html:
     16
    1172022-04-22  Simon Fraser  <simon.fraser@apple.com>
    218

  • trunk/LayoutTests/http/tests/inspector/network/resource-response-source-disk-cache-expected.txt

    r213621 r293264  
    77PASS: Resource should be created.
    88PASS: Resource should receive a Response.
     9PASS: Response `body` should not be empty.
     10PASS: Response should be base64 encoded.
    911
    1012-- Running test case: Resource.ResponseSource.DiskCache
    … …  
    1315PASS: statusCode should be 200
    1416PASS: responseSource should be Symbol(disk-cache)
     17PASS: Response `body` should not be empty.
     18PASS: Response should be base64 encoded.
    1519

  • trunk/LayoutTests/http/tests/inspector/network/resource-response-source-disk-cache.html

    r220119 r293264  
    4141                    if (responseSource)
    4242                        InspectorTest.expectEqual(resource.responseSource, responseSource, `responseSource should be ${String(responseSource)}`);
     43                    return resource.requestContentFromBackend();
     44                }).then((responseContent) => {
     45                    InspectorTest.expectTrue(responseContent.body.length, "Response `body` should not be empty.");
     46                    InspectorTest.expectTrue(responseContent.base64Encoded, "Response should be base64 encoded.");
    4347                }).then(resolve, reject);
    4448            }

  • trunk/LayoutTests/http/tests/inspector/network/resource-response-source-memory-cache-expected.txt

    r225007 r293264  
    77PASS: statusCode should be 200
    88PASS: responseSource should be Symbol(memory-cache)
     9PASS: Response `body` should not be empty.
     10PASS: Response should not be base64 encoded.
    911

  • trunk/LayoutTests/http/tests/inspector/network/resource-response-source-memory-cache.html

    r236766 r293264  
    3333                    InspectorTest.expectEqual(resource.statusCode, statusCode, `statusCode should be ${statusCode}`);
    3434                    InspectorTest.expectEqual(resource.responseSource, responseSource, `responseSource should be ${String(responseSource)}`);
     35                    return resource.requestContentFromBackend();
     36                }).then((responseContent) => {
     37                    InspectorTest.expectTrue(responseContent.body.length, "Response `body` should not be empty.");
     38                    InspectorTest.expectFalse(responseContent.base64Encoded, "Response should not be base64 encoded.");
    3539                }).then(resolve, reject);
    3640            }

  • trunk/Source/WebCore/ChangeLog

    r293260 r293264  
     12022-04-22  Patrick Angle  <pangle@apple.com>
     2
     3        Web Inspector: Regression(r287684) Resources from the memory cache show empty content in Network, Sources, and Search tabs
     4        https://bugs.webkit.org/show_bug.cgi?id=239667
     5
     6        Reviewed by Devin Rousso.
     7
     8        Updated tests:
     9        - http/tests/inspector/network/resource-response-source-disk-cache.html
     10        - http/tests/inspector/network/resource-response-source-memory-cache.html
     11
     12        r287684 introduced a subtle bug when calling InspectorInstrumentation::didReceiveData. We rely
     13        on there being a difference between real, but empty, data buffer and not having a data buffer,
     14        but after r287684 an empty SharedBuffer would be passed in the later case. Instead, we should
     15        continue to pass nullptr if there is no buffer so that InspectorNetworkAgent::didReceiveData
     16        can distiguish between the two.
     17
     18        The bug is the result of having a non-nullptr `data` in `InspectorNetworkAgent::didReceiveData`,
     19        which causes us to call `maybeAddResourceData`, which means by the time
     20        `InspectorNetworkAgent::getResponseBody` is called, the ResourceData for the request will have
     21        an empty, not non-existant, `content()`, which means we will return the empty content instead,
     22        since we believe the response had actual content.
     23
     24        * inspector/InspectorInstrumentation.cpp:
     25        (WebCore::InspectorInstrumentation::didReceiveDataImpl):
     26        * inspector/InspectorInstrumentation.h:
     27        (WebCore::InspectorInstrumentation::didReceiveData):
     28        * loader/ResourceLoadNotifier.cpp:
     29        (WebCore::ResourceLoadNotifier::dispatchDidReceiveData):
     30
    1312022-04-22  Simon Fraser  <simon.fraser@apple.com>
    232

  • trunk/Source/WebCore/inspector/InspectorInstrumentation.cpp

    r290671 r293264  
    629629}
    630630
    631 void InspectorInstrumentation::didReceiveDataImpl(InstrumentingAgents& instrumentingAgents, ResourceLoaderIdentifier identifier, const SharedBuffer& buffer, int encodedDataLength)
    632 {
    633     if (auto* networkAgent = instrumentingAgents.enabledNetworkAgent())
    634         networkAgent->didReceiveData(identifier, &buffer, buffer.size(), encodedDataLength);
     631void InspectorInstrumentation::didReceiveDataImpl(InstrumentingAgents& instrumentingAgents, ResourceLoaderIdentifier identifier, const SharedBuffer* buffer, int encodedDataLength)
     632{
     633    if (auto* networkAgent = instrumentingAgents.enabledNetworkAgent())
     634        networkAgent->didReceiveData(identifier, buffer, buffer ? buffer->size() : 0, encodedDataLength);
    635635}
    636636

  • trunk/Source/WebCore/inspector/InspectorInstrumentation.h

    r290671 r293264  
    199199    static void didReceiveResourceResponse(Frame&, ResourceLoaderIdentifier, DocumentLoader*, const ResourceResponse&, ResourceLoader*);
    200200    static void didReceiveThreadableLoaderResponse(DocumentThreadableLoader&, ResourceLoaderIdentifier);
    201     static void didReceiveData(Frame*, ResourceLoaderIdentifier, const SharedBuffer&, int encodedDataLength);
     201    static void didReceiveData(Frame*, ResourceLoaderIdentifier, const SharedBuffer*, int encodedDataLength);
    202202    static void didFinishLoading(Frame*, DocumentLoader*, ResourceLoaderIdentifier, const NetworkLoadMetrics&, ResourceLoader*);
    203203    static void didFailLoading(Frame*, DocumentLoader*, ResourceLoaderIdentifier, const ResourceError&);
    … …  
    423423    static void didReceiveResourceResponseImpl(InstrumentingAgents&, ResourceLoaderIdentifier, DocumentLoader*, const ResourceResponse&, ResourceLoader*);
    424424    static void didReceiveThreadableLoaderResponseImpl(InstrumentingAgents&, DocumentThreadableLoader&, ResourceLoaderIdentifier);
    425     static void didReceiveDataImpl(InstrumentingAgents&, ResourceLoaderIdentifier, const SharedBuffer&, int encodedDataLength);
     425    static void didReceiveDataImpl(InstrumentingAgents&, ResourceLoaderIdentifier, const SharedBuffer*, int encodedDataLength);
    426426    static void didFinishLoadingImpl(InstrumentingAgents&, ResourceLoaderIdentifier, DocumentLoader*, const NetworkLoadMetrics&, ResourceLoader*);
    427427    static void didFailLoadingImpl(InstrumentingAgents&, ResourceLoaderIdentifier, DocumentLoader*, const ResourceError&);
    … …  
    11051105}
    11061106   
    1107 inline void InspectorInstrumentation::didReceiveData(Frame* frame, ResourceLoaderIdentifier identifier, const SharedBuffer& buffer, int encodedDataLength)
     1107inline void InspectorInstrumentation::didReceiveData(Frame* frame, ResourceLoaderIdentifier identifier, const SharedBuffer* buffer, int encodedDataLength)
    11081108{
    11091109    FAST_RETURN_IF_NO_FRONTENDS(void());
    … …  
    11151115{
    11161116    FAST_RETURN_IF_NO_FRONTENDS(void());
    1117     didReceiveDataImpl(instrumentingAgents(globalScope), identifier, buffer, buffer.size());
     1117    didReceiveDataImpl(instrumentingAgents(globalScope), identifier, &buffer, buffer.size());
    11181118}
    11191119

  • trunk/Source/WebCore/loader/ResourceLoadNotifier.cpp

    r287684 r293264  
    161161    m_frame.loader().client().dispatchDidReceiveContentLength(loader, identifier, expectedDataLength);
    162162
    163     InspectorInstrumentation::didReceiveData(&m_frame, identifier, buffer ? *buffer : SharedBuffer::create(), encodedDataLength);
     163    InspectorInstrumentation::didReceiveData(&m_frame, identifier, buffer, encodedDataLength);
    164164}
    165165
Note: See TracChangeset for help on using the changeset viewer.