Include requests with no response in NetworkRequests handler

Requests that never send a response are a thing that happens:

* Long-polling requests that never end
* The trace ended before the request did

We should not exclude these from the trace processor. This helps with
Lantern - see GoogleChrome/lighthouse#16492.

Additionally, I think this handler wanted to filter out data urls, but
it wasn't doing it directly so some got through. That should be fixed
now.

Bug: none
Change-Id: I911211512055667fa2deb125dd64389ce8968324
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6569818
Auto-Submit: Connor Clark <cjamcl@chromium.org>
Reviewed-by: Paul Irish <paulirish@chromium.org>
Commit-Queue: Connor Clark <cjamcl@chromium.org>

Author: Connor Clark

front_end/models/ai_assistance/data_formatters/PerformanceInsightFormatter.ts

@@ -495,6 +495,6 @@ MIME Type: ${mimeType}
 ${priorityLines.join('\n')}
 Render blocking: ${renderBlocking ? 'Yes' : 'No'}
 From a service worker: ${fromServiceWorker ? 'Yes' : 'No'}
-${NetworkRequestFormatter.formatHeaders('Response headers', responseHeaders, true)}`;
+${NetworkRequestFormatter.formatHeaders('Response headers', responseHeaders ?? [], true)}`;
   }
 }

front_end/models/trace/LanternComputationData.ts

@@ -87,7 +87,7 @@ function findWorkerThreads(trace: Lantern.Types.Trace): Map<number, number[]> {
 function createLanternRequest(
     parsedTrace: Readonly<Handlers.Types.ParsedTrace>, workerThreads: Map<number, number[]>,
     request: Types.Events.SyntheticNetworkRequest): NetworkRequest|undefined {
-  if (request.args.data.connectionId === undefined || request.args.data.connectionReused === undefined) {
+  if (request.args.data.hasResponse && request.args.data.connectionId === undefined) {
     throw new Lantern.Core.LanternError('Trace is too old');
   }
 
@@ -166,8 +166,8 @@ function createLanternRequest(
   return {
     rawRequest: request,
     requestId: request.args.data.requestId,
-    connectionId: request.args.data.connectionId,
-    connectionReused: request.args.data.connectionReused,
+    connectionId: request.args.data.connectionId ?? 0,
+    connectionReused: request.args.data.connectionReused ?? false,
     url: request.args.data.url,
     protocol: request.args.data.protocol,
     parsedURL: createParsedUrl(url),

front_end/models/trace/handlers/NetworkRequestsHandler.ts

@@ -106,9 +106,9 @@ function storeTraceEventWithRequestId<K extends keyof TraceEventsForNetworkReque
   }
 }
 
-function firstPositiveValueInList(entries: number[]): number {
+function firstPositiveValueInList(entries: Array<number|null>): number {
   for (const entry of entries) {
-    if (entry > 0) {
+    if (entry && entry > 0) {
       return entry;
     }
   }
@@ -204,7 +204,7 @@ export async function finalize(): Promise<void> {
   for (const [requestId, request] of requestMap.entries()) {
     // If we have an incomplete set of events here, we choose to drop the network
     // request rather than attempt to synthesize the missing data.
-    if (!request.sendRequests || !request.receiveResponse) {
+    if (!request.sendRequests) {
       continue;
     }
 
@@ -244,12 +244,20 @@ export async function finalize(): Promise<void> {
       });
     }
 
+    const firstSendRequest = request.sendRequests[0];
+    const finalSendRequest = request.sendRequests[request.sendRequests.length - 1];
+
+    // We currently do not want to include data URI requests. We may revisit this in the future.
+    if (finalSendRequest.args.data.url.startsWith('data:')) {
+      continue;
+    }
+
     // If a ResourceFinish event with an encoded data length is received,
     // then the resource was not cached; it was fetched before it was
     // requested, e.g. because it was pushed in this navigation.
     const isPushedResource = request.resourceFinish?.args.data.encodedDataLength !== 0;
     // This works around crbug.com/998397, which reports pushed resources, and resources served by a service worker as disk cached.
-    const isDiskCached = request.receiveResponse.args.data.fromCache &&
+    const isDiskCached = !!request.receiveResponse && request.receiveResponse.args.data.fromCache &&
         !request.receiveResponse.args.data.fromServiceWorker && !isPushedResource;
     // If the request contains a resourceMarkAsCached event, it was served from memory cache.
     // The timing data returned is from the original (uncached) request, which
@@ -265,15 +273,12 @@ export async function finalize(): Promise<void> {
     const isMemoryCached = request.resourceMarkAsCached !== undefined;
     // If a request has `resourceMarkAsCached` field, the `timing` field is not correct.
     // So let's discard it and override to 0 (which will be handled in later logic if timing field is undefined).
-    const timing = isMemoryCached ? undefined : request.receiveResponse.args.data.timing;
-    // If a non-cached request has no |timing| indicates data URLs, we ignore it.
-    if (!timing && !isMemoryCached) {
+    const timing = isMemoryCached ? undefined : request.receiveResponse?.args.data.timing;
+    // If a non-cached response has no |timing|, we ignore it. An example of this is chrome://new-page / about:blank.
+    if (request.receiveResponse && !timing && !isMemoryCached) {
       continue;
     }
 
-    const firstSendRequest = request.sendRequests[0];
-    const finalSendRequest = request.sendRequests[request.sendRequests.length - 1];
-
     const initialPriority = finalSendRequest.args.data.priority;
     let finalPriority = initialPriority;
     if (request.changePriority) {
@@ -343,13 +348,14 @@ export async function finalize(): Promise<void> {
     // Otherwise it is whichever positive number comes first from the following timing info:
     // DNS start, Connection start, Send Start, or the time duration between our start time and
     // receiving a response.
-    const stalled = timing ? Types.Timing.Micro(firstPositiveValueInList([
-      timing.dnsStart * MILLISECONDS_TO_MICROSECONDS,
-      timing.connectStart * MILLISECONDS_TO_MICROSECONDS,
-      timing.sendStart * MILLISECONDS_TO_MICROSECONDS,
-      (request.receiveResponse.ts - endRedirectTime),
-    ])) :
-                             Types.Timing.Micro(request.receiveResponse.ts - startTime);
+    const stalled = timing ?
+        Types.Timing.Micro(firstPositiveValueInList([
+          timing.dnsStart * MILLISECONDS_TO_MICROSECONDS,
+          timing.connectStart * MILLISECONDS_TO_MICROSECONDS,
+          timing.sendStart * MILLISECONDS_TO_MICROSECONDS,
+          request.receiveResponse ? (request.receiveResponse.ts - endRedirectTime) : null,
+        ])) :
+        (request.receiveResponse ? Types.Timing.Micro(request.receiveResponse.ts - startTime) : Types.Timing.Micro(0));
 
     // Sending HTTP request
     // =======================
@@ -373,8 +379,9 @@ export async function finalize(): Promise<void> {
         Types.Timing.Micro(
             timing.requestTime * SECONDS_TO_MICROSECONDS + timing.receiveHeadersEnd * MILLISECONDS_TO_MICROSECONDS) :
         startTime;
-    const download = timing ? Types.Timing.Micro(((finishTime || downloadStart) - downloadStart)) :
-                              Types.Timing.Micro(endTime - request.receiveResponse.ts);
+    const download = timing     ? Types.Timing.Micro(((finishTime || downloadStart) - downloadStart)) :
+        request.receiveResponse ? Types.Timing.Micro(endTime - request.receiveResponse.ts) :
+                                  Types.Timing.Micro(0);
 
     const totalTime = Types.Timing.Micro(networkDuration + processingDuration);
 
@@ -435,30 +442,31 @@ export async function finalize(): Promise<void> {
               decodedBodyLength,
               encodedDataLength,
               frame,
-              fromServiceWorker: request.receiveResponse.args.data.fromServiceWorker,
+              fromServiceWorker: request.receiveResponse?.args.data.fromServiceWorker,
               isLinkPreload: finalSendRequest.args.data.isLinkPreload || false,
-              mimeType: request.receiveResponse.args.data.mimeType,
+              mimeType: request.receiveResponse?.args.data.mimeType ?? '',
               priority: finalPriority,
               initialPriority,
-              protocol: request.receiveResponse.args.data.protocol ?? 'unknown',
+              protocol: request.receiveResponse?.args.data.protocol ?? 'unknown',
               redirects,
               // In the event the property isn't set, assume non-blocking.
               renderBlocking: renderBlocking ?? 'non_blocking',
               requestId,
               requestingFrameUrl,
               requestMethod: finalSendRequest.args.data.requestMethod,
               resourceType: finalSendRequest.args.data.resourceType ?? Protocol.Network.ResourceType.Other,
-              statusCode: request.receiveResponse.args.data.statusCode,
-              responseHeaders: request.receiveResponse.args.data.headers || [],
+              statusCode: request.receiveResponse?.args.data.statusCode ?? 0,
+              responseHeaders: request.receiveResponse?.args.data.headers ?? null,
               fetchPriorityHint: finalSendRequest.args.data.fetchPriorityHint ?? 'auto',
               initiator: finalSendRequest.args.data.initiator,
               stackTrace: finalSendRequest.args.data.stackTrace,
               timing,
               url,
               failed: request.resourceFinish?.args.data.didFail ?? false,
               finished: Boolean(request.resourceFinish),
-              connectionId: request.receiveResponse.args.data.connectionId,
-              connectionReused: request.receiveResponse.args.data.connectionReused,
+              hasResponse: Boolean(request.receiveResponse),
+              connectionId: request.receiveResponse?.args.data.connectionId,
+              connectionReused: request.receiveResponse?.args.data.connectionReused,
             },
           },
           cat: 'loading',

front_end/models/trace/insights/Cache.ts

@@ -197,7 +197,7 @@ export function generateInsight(
   let totalWastedBytes = 0;
   const wastedBytesByRequestId = new Map<string, number>();
   for (const req of contextRequests) {
-    if (!isCacheable(req)) {
+    if (!req.args.data.responseHeaders || !isCacheable(req)) {
       continue;
     }
 

front_end/models/trace/insights/Common.ts

@@ -300,6 +300,10 @@ export function metricSavingsForWastedBytes(
  * Returns whether the network request was sent encoded.
  */
 export function isRequestCompressed(request: Types.Events.SyntheticNetworkRequest): boolean {
+  if (!request.args.data.responseHeaders) {
+    return false;
+  }
+
   // FYI: In Lighthouse, older devtools logs (like our test fixtures) seems to be
   // lower case, while modern logs are Cased-Like-This.
   const patterns = [

front_end/models/trace/lantern/core/NetworkAnalyzer.test.ts

@@ -161,8 +161,8 @@ describe('NetworkAnalyzer', () => {
       const requests = await createRequests(trace);
       const result = NetworkAnalyzer.estimateIfConnectionWasReused(requests);
       const distinctConnections = Array.from(result.values()).filter(item => !item).length;
-      assert.strictEqual(result.size, 25);
-      assert.strictEqual(distinctConnections, 9);
+      assert.strictEqual(result.size, 24);
+      assert.strictEqual(distinctConnections, 8);
     });
   });
 

front_end/models/trace/lantern/metrics/Interactive.test.ts

@@ -38,7 +38,7 @@ describe('Metrics: Lantern TTI', function() {
           timing: 1122,
         });
     assert.strictEqual(result.optimisticEstimate.nodeTimings.size, 14);
-    assert.strictEqual(result.pessimisticEstimate.nodeTimings.size, 31);
+    assert.strictEqual(result.pessimisticEstimate.nodeTimings.size, 29);
     assert.isOk(result.optimisticGraph, 'should have created optimistic graph');
     assert.isOk(result.pessimisticGraph, 'should have created pessimistic graph');
   });

front_end/models/trace/lantern/metrics/LargestContentfulPaint.test.ts

@@ -30,11 +30,11 @@ describe('Metrics: Lantern LCP', function() {
           pessimisticNodeTimings: result.pessimisticEstimate.nodeTimings.size,
         },
         {
-          timing: 1536,
+          timing: 1457,
           optimistic: 1457,
-          pessimistic: 1616,
+          pessimistic: 1457,
           optimisticNodeTimings: 8,
-          pessimisticNodeTimings: 9,
+          pessimisticNodeTimings: 8,
         });
     assert.isOk(result.optimisticGraph, 'should have created optimistic graph');
     assert.isOk(result.pessimisticGraph, 'should have created pessimistic graph');

front_end/models/trace/types/TraceEvents.ts

@@ -373,8 +373,9 @@ export interface SyntheticNetworkRequest extends Complete, SyntheticBased<Phase.
        */
       encodedDataLength: number,
       frame: string,
-      fromServiceWorker: boolean,
+      fromServiceWorker: boolean|undefined,
       isLinkPreload: boolean,
+      /** Empty string if no response. */
       mimeType: string,
       priority: Protocol.Network.ResourcePriority,
       initialPriority: Protocol.Network.ResourcePriority,
@@ -390,17 +391,21 @@ export interface SyntheticNetworkRequest extends Complete, SyntheticBased<Phase.
       renderBlocking: RenderBlocking,
       requestId: string,
       requestingFrameUrl: string,
+      /** 0 if no response. */
       statusCode: number,
       resourceType: Protocol.Network.ResourceType,
-      responseHeaders: Array<{name: string, value: string}>,
+      responseHeaders: Array<{name: string, value: string}>|null,
       fetchPriorityHint: FetchPriorityHint,
       url: string,
       /** True only if got a 'resourceFinish' event indicating a failure. */
       failed: boolean,
-      /** True only if got a 'resourceFinish' event. */
+      /** True only if got a 'resourceFinish' event. Note even failed requests with no response may be "finished". */
       finished: boolean,
-      connectionId: number,
-      connectionReused: boolean,
+      hasResponse: boolean,
+      /** If undefined, trace was either too old or had no response. */
+      connectionId: number|undefined,
+      /** If undefined, trace was either too old or had no response. */
+      connectionReused: boolean|undefined,
       // Optional fields
       initiator?: Initiator,
       requestMethod?: string,

front_end/panels/timeline/TimelineTreeView.test.ts

@@ -293,14 +293,12 @@ describeWithEnvironment('TimelineTreeView', function() {
         'https://web-dev.imgix.net/image/kheDArv5csY6rvQUJDbWRscckLr1/4i7JstVZvgTFk9dxCe4a.svg',
         'https://web-dev.imgix.net/image/jxu1OdD7LKOGIDU7jURMpSH2lyK2/zrBPJq27O4Hs8haszVnK.svg',
         'https://web-dev.imgix.net/image/jL3OLOhcWUQDnR4XjewLBx4e3PC3/3164So5aDk7vKTkhx9Vm.png?auto=format&w=1140',
-        'data:image/svg+xml,%3Csvg viewBox=\'0 0 18 18\' fill=\'%23191919\' xmlns=\'http://www.w3.org/2000/svg\'%3E%3Cpath fill-rule=\'evenodd\' clip-rule=\'evenodd\' d=\'M16 2V16H2V2H16ZM16 0H2C0.9 0 0 0.9 0 2V16C0 17.1 0.9 18 2 18H16C17.1 18 18 17.1 18 16V2C18 0.9 17.1 0 16 0Z\' /%3E%3C/svg%3E',
         'https://web.dev/js/app.js?v=fedf5fbe',
         'https://web.dev/js/home.js?v=73b0d143',
         'https://web.dev/js/index-7e29abb6.js',
         'https://web.dev/js/index-578d2db7.js',
         'https://web.dev/js/actions-f0eb5c8e.js',
         'https://web.dev/js/index-f45448ab.js',
-        'data:image/svg+xml,%3Csvg width=\'24\' height=\'24\' viewBox=\'0 0 24 24\' fill=\'none\' xmlns=\'http://www.w3.org/2000/svg\'%3E%3Cpath d=\'M7 10L12 15L17 10H7Z\' fill=\'%235F6368\'/%3E%3C/svg%3E%0A',
         'https://www.googletagmanager.com/gtm.js?id=GTM-MZWCJPP',
         'https://www.google-analytics.com/analytics.js',
         'https://www.googletagmanager.com/gtag/js?id=G-18JR3Q8PJ8&l=dataLayer&cx=c',