Bug 150005 – Web Inspector: show redirect requests in Network and Timelines tabs

• Component: Web Inspector
• Status: RESOLVED FIXED

[André Arko]

In previous versions of Safari, redirect requests were displayed as entries in the "Resources" pane. Today, neither the "Network" or "Timeline" tabs show redirects, instead clearing the entire list of entries any time a redirect occurs, throwing away all network request information up to and including the redirect.

In older versions of Safari, showing redirect requests was considered so important that it merited its own section in an announcement on the Webkit blog: https://www.webkit.org/blog/1091/more-web-inspector-updates/#resources_inspection.

This regression is the entire reason I have to use Chrome to do web development at my job. :/

[Radar WebKit Bug Importer]

<rdar://problem/23061686>

[Timothy Hatcher]

<rdar://problem/5378164>

[Devin Rousso]

Created attachment 351056 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 351057 [details]
[Image] After Patch is applied

[EWS Watchlist]

Attachment 351056 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 351056 [details]
[Patch] WIP

Attachment 351056 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9378407

New failing tests:
http/tests/inspector/network/har/har-basic.html

[EWS Watchlist]

Created attachment 351062 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 351056 [details]
[Patch] WIP

Attachment 351056 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9378427

New failing tests:
http/tests/inspector/network/har/har-basic.html

[EWS Watchlist]

Created attachment 351063 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 351056 [details]
[Patch] WIP

Attachment 351056 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9378480

New failing tests:
http/tests/inspector/network/har/har-basic.html

[EWS Watchlist]

Created attachment 351065 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 351108 [details]
Patch

Added logic to preserve old (incorrect) behaviour when redirects are present

[Joseph Pecoraro]

Comment on attachment 351056 [details]
[Patch] WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=351056&action=review

Some notes:

• I'd like to see a screenshot of the Headers section
• The timing breakdown could show each of the individual redirects (multiple gray boxes/borders) instead of a single gray box, so that if there are multiple redirects it would be immediately apparent.
• We should probably include redirect data in the Timeline tab (but maybe that should be a follow-up since I think including it in the Network tab is more important).

Other browsers include each individual redirect as a separate row in their Network Table. That allows for filtering/searching and finding all of the "301" responses quite quickly. I prefer our approach of making the Network Table more about resources. Thus a single row can group multiple requests for the same Resource. Ultimately things like redirects for a resource, and CORS pre-flight requests for a resource would all end up on a single row instead of multiple rows. But I'm wondering if there are any disadvantages we may be overlooking.

> Source/JavaScriptCore/ChangeLog:3
> +        REGRESSION: Web Inspector: Redirect requests are not shown in either Network or Timeline tabs

It may be worth retitling this bug. I don't think REGRESSION is very apt anymore and this mentions Timeline tab which I don't think is being addressed in this patch.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:517
> +        MonotonicTime fetchStart = resourceLoader->loadTiming().fetchStart();
> +        Seconds fetchStartInInspector = m_environment.executionStopwatch()->elapsedTimeSince(fetchStart);
> +        elapsedFinishTime = (fetchStartInInspector + networkLoadMetrics.responseEnd).seconds();

If this is so different the frontend will need to be backwards compatible with older backends when showing redirects.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:687
> +        this._redirects.push({

I'd rather see a WI.Redirect model object but I could be convinced of just leaving it a plain object.

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:70
> +        this._refreshRedirectHeadersSections();

As mentioned in person I'd like to see the Summary section update as well to show the chain of redirects.

One possible appearance would be:

    URL: https://out.example/foo/bar
      ➞: https://trampoline.example/redirect/foo/bar
      ➞: https://foo.example/bar

But I think showing the full chain of URLs close together would be enlightening and useful for users.

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:93
> +        this._needsRedirectHeadersRefresh = false;

This member `_needsRedirectHeadersRefresh` is never set to true. It probably should be at some point in order to update that section while WI.Resource gets redirects.

An example scenario in which this applies:

1. Start a resource load
2. Open the Resource's Headers Detail view in Web Inspector
3. Watch it get redirected once
  => Should see a set of Request + Redirect Response sections show up but still loading
4. Watch it get redirected again
  => Should see another set of Request + Redirect Response sections show up but still loading
5. Watch the request complete
  => Should see Response section show up

Likewise in these situations the Request section itself should probably update since it will have a new URL? I feel like we're missing an update event.

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:301
> +            let redirectRequestSection = new WI.ResourceDetailsSection(WI.UIString("Request"), "redirect");

Something we didn't try that might be worth trying would be a yellow/orange color for redirects instead of the usual blue. It might be too jarring but I think its worth trying out a different color.

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:313
> +            this._appendKeyValuePair(redirectResponseSection.detailsElement, `${redirect.responseStatusCode} ${redirect.responseStatusText}`, null, "h1-status");

Hmm so this always outputs "h1-status". That may be misleading if the request/response were HTTP/2. Have we tested an HTTP/2 redirect, which I'd expect a :status header. I realize here you're synthesizing data, and that may be fine but it would be nice to get as close to the raw transcript as possible.

> Source/WebInspectorUI/UserInterface/Views/Variables.css:110
> +    --network-redirect-color: lightgrey;

Nice! I love gray. Does this work well in Dark Mode?

[Devin Rousso]

Comment on attachment 351056 [details]
[Patch] WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=351056&action=review

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:517
>> +        elapsedFinishTime = (fetchStartInInspector + networkLoadMetrics.responseEnd).seconds();
> 
> If this is so different the frontend will need to be backwards compatible with older backends when showing redirects.

The new patch (https://bugs.webkit.org/attachment.cgi?id=351108) ensures that the functionality for older backends is unchanged, which is slightly unfortunate given that the other time values are wrong for redirected requests.  I tried seeing if there was a way to deduce the redirect timing information from what we previously sent to the frontend, but there seems to be a disconnect between the `elapsedTime` and `timingData` values, as they  both seem to use different systems (`elapsedTime` is from `m_environment.executionStopwatch()->elapsedTimeSince(...)` whereas `timingData` is just a plain `MonotonicTime`).

>> Source/WebInspectorUI/UserInterface/Models/Resource.js:687
>> +        this._redirects.push({
> 
> I'd rather see a WI.Redirect model object but I could be convinced of just leaving it a plain object.

Agreed.

>> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:93
>> +        this._needsRedirectHeadersRefresh = false;
> 
> This member `_needsRedirectHeadersRefresh` is never set to true. It probably should be at some point in order to update that section while WI.Resource gets redirects.
> 
> An example scenario in which this applies:
> 
> 1. Start a resource load
> 2. Open the Resource's Headers Detail view in Web Inspector
> 3. Watch it get redirected once
>   => Should see a set of Request + Redirect Response sections show up but still loading
> 4. Watch it get redirected again
>   => Should see another set of Request + Redirect Response sections show up but still loading
> 5. Watch the request complete
>   => Should see Response section show up
> 
> Likewise in these situations the Request section itself should probably update since it will have a new URL? I feel like we're missing an update event.

Oh crap I forgot about this part 😅

>> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:301
>> +            let redirectRequestSection = new WI.ResourceDetailsSection(WI.UIString("Request"), "redirect");
> 
> Something we didn't try that might be worth trying would be a yellow/orange color for redirects instead of the usual blue. It might be too jarring but I think its worth trying out a different color.

My fear with a color like that is that it might be too close to the red we use for error.  I can give it a try though.

>> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:313
>> +            this._appendKeyValuePair(redirectResponseSection.detailsElement, `${redirect.responseStatusCode} ${redirect.responseStatusText}`, null, "h1-status");
> 
> Hmm so this always outputs "h1-status". That may be misleading if the request/response were HTTP/2. Have we tested an HTTP/2 redirect, which I'd expect a :status header. I realize here you're synthesizing data, and that may be fine but it would be nice to get as close to the raw transcript as possible.

I did this because we don't actually get any protocol information from a redirect request/response, so I tried to do the next best thing, which was to just not show it at all.  Is it possible for the protocol to change per-request?  If not, I can just use the protocol from the first request.

>> Source/WebInspectorUI/UserInterface/Views/Variables.css:110
>> +    --network-redirect-color: lightgrey;
> 
> Nice! I love gray. Does this work well in Dark Mode?

...totally forgot to test that 🤦

[Devin Rousso]

Created attachment 351420 [details]
Patch

[Blaze Burg]

Comment on attachment 351420 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351420&action=review

This looks good overall, but it no longer applies to TOT so I couldn't test it out.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:201
> +    auto elapsedTimeSince = [&] (const MonotonicTime& time) {

Nice.

[Joseph Pecoraro]

Comment on attachment 351420 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351420&action=review

Oops, I missed that there was a new version of the patch up. This patch looks good to me.

> Source/WebInspectorUI/UserInterface/Models/ResourceTimingData.js:68
> +        // COMPATIBILITY (iOS 12.1): Resource Timing data was based on startTime, not fetchStart.

I think we would normally put (iOS 12.0) here.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:-145
> -    min-width: 3px;
> -    -webkit-margin-start: -1px;

What is this being replaced with? This provided a minimum size to the network graphs, it seems we would still want this unless I missed how it is getting replaced.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:577
> +            appendBlock(domainLookupStart, domainLookupEnd || connectStart || requestStart, "dns");

I think it was intentional here to go from domainLookupStart to connectStart if not just for appearances. IF there is a gap between `domainLookupEnd` and `connectStart` it looks weird / poor.

> LayoutTests/http/tests/inspector/network/resource-timing.html:21
> +    InspectorTest.debug();

Oop

[Devin Rousso]

Created attachment 351829 [details]
Patch

Rebase

[Devin Rousso]

Created attachment 351837 [details]
Patch

Added small "filler" blocks when timestamps don't perfectly align

[Devin Rousso]

Created attachment 351838 [details]
[Image] After Patch is applied

[EWS Watchlist]

Comment on attachment 351837 [details]
Patch

Attachment 351837 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9497467

New failing tests:
inspector/canvas/recording.html

[EWS Watchlist]

Created attachment 351844 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Joseph Pecoraro]

Comment on attachment 351837 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351837&action=review

r=me!

> Source/JavaScriptCore/inspector/protocol/Network.json:44
>                  { "name": "startTime", "type": "number", "description": "Timing's startTime is a baseline in seconds, while the other numbers are ticks in milliseconds relatively to this." },
> +                { "name": "redirectStart", "type": "number", "description": "Started redirect resolution." },
> +                { "name": "redirectEnd", "type": "number", "description": "Finished redirect resolution." },
> +                { "name": "fetchStart", "type": "number", "description": "Started fetching the resource." },

I think you need to add something to the description of these.

• The description of fetchStart should be like what startTime is now.
• The description of startTime/redirectStart/redirectEnd should point out that they are in seconds, and that they are not milliseconds relative to something else.

We will want to email our ITML friends about this change in required properties on the Network.ResourceTiming object. I think they make use of it.

> Source/WebInspectorUI/UserInterface/Models/ResourceTimingData.js:81
> +            redirectStart: payload.redirectStart,
> +            redirectEnd: payload.redirectEnd,

If these are undefined (legacy backed) or zero (invalid value), should we just make them NaN immediately?

    redirectStart: payload.redirectStart || NaN,

We may even want to assert that redirectStart/End are after startTime and before fetchStart, otherwise fallback to NaN because its invalid data.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:217
> +.waterfall .block.filler{

Style: Space before brace.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:687
> +            if (startTimestamp > lastEndTimestamp)
> +                appendBlock(lastEndTimestamp, startTimestamp, "filler");
> +            lastEndTimestamp = endTimestamp;

I figured we'd just make 1 filler block in the background from start to end and draw all the other blocks on top of it. That seems simpler then creating a bunch of filler blocks in the middle.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:727
>          if (connectStart)
> -            appendBlock(connectStart, connectEnd, "connect");
> +            appendBlock(connectStart, secureConnectionStart || connectEnd, "connect");

I think this one may be unnecessary since secureConnectionStart is in the middle of connectStart/connectEnd. In ResourceTiming connectStart to connectEnd includes the secure and leading time anyways.

[Devin Rousso]

Comment on attachment 351837 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351837&action=review

>> Source/WebInspectorUI/UserInterface/Models/ResourceTimingData.js:81
>> +            redirectEnd: payload.redirectEnd,
> 
> If these are undefined (legacy backed) or zero (invalid value), should we just make them NaN immediately?
> 
>     redirectStart: payload.redirectStart || NaN,
> 
> We may even want to assert that redirectStart/End are after startTime and before fetchStart, otherwise fallback to NaN because its invalid data.

Considering that they get reset to `NaN` in the constructor, and that `NaN` is falsy, it seems weird to assign it here only to have it be reassigned as `NaN` again.  Adding the assertions makes sense.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:687
>> +            lastEndTimestamp = endTimestamp;
> 
> I figured we'd just make 1 filler block in the background from start to end and draw all the other blocks on top of it. That seems simpler then creating a bunch of filler blocks in the middle.

I personally prefer not to have overlapping DOM nodes, as it can create some annoying roadblocks if changes are needed in the future.  Another reason I did it this way was to facilitate <https://webkit.org/b/190379>, but that could also just apply the `disjoint` class where we would create a "filler" block.  I'll rework this.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:727
>> +            appendBlock(connectStart, secureConnectionStart || connectEnd, "connect");
> 
> I think this one may be unnecessary since secureConnectionStart is in the middle of connectStart/connectEnd. In ResourceTiming connectStart to connectEnd includes the secure and leading time anyways.

Ditto about overlapping (687).

[Joseph Pecoraro]

Comment on attachment 351837 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351837&action=review

>>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:687
>>> +            lastEndTimestamp = endTimestamp;
>> 
>> I figured we'd just make 1 filler block in the background from start to end and draw all the other blocks on top of it. That seems simpler then creating a bunch of filler blocks in the middle.
> 
> I personally prefer not to have overlapping DOM nodes, as it can create some annoying roadblocks if changes are needed in the future.  Another reason I did it this way was to facilitate <https://webkit.org/b/190379>, but that could also just apply the `disjoint` class where we would create a "filler" block.  I'll rework this.

Overlapping avoids the potential for rounding issues when edges fall on sub-pixel coordinates.

[Devin Rousso]

Created attachment 351932 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 351932 [details]
Patch

Clearing flags on attachment: 351932

Committed r236995: <https://trac.webkit.org/changeset/236995>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David]

Hi. Just found this issue, I think this has suffered from a rollback in Safari 17.4. When there are too many redirects, Network and Timeline tabs don't display the requests bouncing and thus makes it impossible to debug in the inspector. I feel like it was working before in Webkit but I might be wrong and it could be an exception that fell through the cracks. The only thing showing in the Network and Timeline tabs, even with "preserve logs" activated, are the requests to ErrorPage.html and page-load-errors.css

[Alexey Proskuryakov]

Thank you! Could you please file a new bug with all the details and exact steps to reproduce? 

It is helpful to relate to older bugs, but they won't be used for fixing new issues, definitely not after 6 years.

[David]

Absolutely, thanks for the tip!