WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
150005
Web Inspector: show redirect requests in Network and Timelines tabs
https://bugs.webkit.org/show_bug.cgi?id=150005
Summary
Web Inspector: show redirect requests in Network and Timelines tabs
André Arko
Reported
2015-10-10 15:34:33 PDT
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. :/
Attachments
[Patch] WIP
(35.01 KB, patch)
2018-09-27 23:33 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
[Image] After Patch is applied
(103.58 KB, image/png)
2018-09-27 23:33 PDT
,
Devin Rousso
no flags
Details
Archive of layout-test-results from ews102 for mac-sierra
(2.32 MB, application/zip)
2018-09-28 00:38 PDT
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews104 for mac-sierra-wk2
(2.92 MB, application/zip)
2018-09-28 00:48 PDT
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews114 for mac-sierra
(3.04 MB, application/zip)
2018-09-28 01:32 PDT
,
EWS Watchlist
no flags
Details
Patch
(41.46 KB, patch)
2018-09-28 14:24 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(48.08 KB, patch)
2018-10-02 11:31 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(48.99 KB, patch)
2018-10-08 16:19 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(49.76 KB, patch)
2018-10-08 17:27 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
[Image] After Patch is applied
(75.28 KB, image/png)
2018-10-08 17:27 PDT
,
Devin Rousso
no flags
Details
Archive of layout-test-results from ews102 for mac-sierra
(2.35 MB, application/zip)
2018-10-08 18:33 PDT
,
EWS Watchlist
no flags
Details
Patch
(51.14 KB, patch)
2018-10-09 18:06 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2015-10-10 15:34:46 PDT
<
rdar://problem/23061686
>
Timothy Hatcher
Comment 2
2015-12-12 19:05:44 PST
<
rdar://problem/5378164
>
Devin Rousso
Comment 3
2018-09-27 23:33:25 PDT
Created
attachment 351056
[details]
[Patch] WIP
Devin Rousso
Comment 4
2018-09-27 23:33:48 PDT
Created
attachment 351057
[details]
[Image] After Patch is applied
EWS Watchlist
Comment 5
2018-09-27 23:36:15 PDT
Comment hidden (obsolete)
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 6
2018-09-28 00:38:46 PDT
Comment hidden (obsolete)
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
Comment 7
2018-09-28 00:38:48 PDT
Comment hidden (obsolete)
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 8
2018-09-28 00:48:51 PDT
Comment hidden (obsolete)
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
Comment 9
2018-09-28 00:48:53 PDT
Comment hidden (obsolete)
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 10
2018-09-28 01:32:45 PDT
Comment hidden (obsolete)
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
Comment 11
2018-09-28 01:32:47 PDT
Comment hidden (obsolete)
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
Comment 12
2018-09-28 14:24:53 PDT
Created
attachment 351108
[details]
Patch Added logic to preserve old (incorrect) behaviour when redirects are present
Joseph Pecoraro
Comment 13
2018-09-28 14:53:06 PDT
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 14
2018-09-28 15:07:44 PDT
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
Comment 15
2018-10-02 11:31:09 PDT
Created
attachment 351420
[details]
Patch
Blaze Burg
Comment 16
2018-10-08 12:42:07 PDT
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 17
2018-10-08 14:02:25 PDT
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
Comment 18
2018-10-08 16:19:37 PDT
Created
attachment 351829
[details]
Patch Rebase
Devin Rousso
Comment 19
2018-10-08 17:27:16 PDT
Created
attachment 351837
[details]
Patch Added small "filler" blocks when timestamps don't perfectly align
Devin Rousso
Comment 20
2018-10-08 17:27:37 PDT
Created
attachment 351838
[details]
[Image] After Patch is applied
EWS Watchlist
Comment 21
2018-10-08 18:33:14 PDT
Comment hidden (obsolete)
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
Comment 22
2018-10-08 18:33:16 PDT
Comment hidden (obsolete)
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 23
2018-10-09 00:30:19 PDT
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 24
2018-10-09 10:00:14 PDT
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 25
2018-10-09 10:30:24 PDT
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
Comment 26
2018-10-09 18:06:44 PDT
Created
attachment 351932
[details]
Patch
WebKit Commit Bot
Comment 27
2018-10-09 19:49:59 PDT
Comment on
attachment 351932
[details]
Patch Clearing flags on attachment: 351932 Committed
r236995
: <
https://trac.webkit.org/changeset/236995
>
WebKit Commit Bot
Comment 28
2018-10-09 19:50:02 PDT
All reviewed patches have been landed. Closing bug.
David
Comment 29
2024-03-21 07:07:45 PDT
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
Comment 30
2024-03-21 08:26:41 PDT
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
Comment 31
2024-03-21 08:28:10 PDT
Absolutely, thanks for the tip!
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug