fix(trace): make sure the correct attachment name is used for downloads (#31928)

When two attachments have the same content sha1, we used the first one's
name for the downloaded file, no matter which one the user clicked to
download. Now we pass the name explicitly.

References #31912.

packages/trace-viewer/src/sw.ts

@@ -130,13 +130,12 @@ async function doFetch(event: FetchEvent): Promise<Response> {
     }
 
     if (relativePath.startsWith('/sha1/')) {
-      const download = url.searchParams.has('download');
       // Sha1 for sources is based on the file path, can't load it of a random model.
       const sha1 = relativePath.slice('/sha1/'.length);
       for (const trace of loadedTraces.values()) {
         const blob = await trace.traceModel.resourceForSha1(sha1);
         if (blob)
-          return new Response(blob, { status: 200, headers: download ? downloadHeadersForAttachment(trace.traceModel, sha1) : undefined });
+          return new Response(blob, { status: 200, headers: downloadHeaders(url.searchParams) });
       }
       return new Response(null, { status: 404 });
     }
@@ -157,14 +156,15 @@ async function doFetch(event: FetchEvent): Promise<Response> {
   return snapshotServer.serveResource(lookupUrls, request.method, snapshotUrl);
 }
 
-function downloadHeadersForAttachment(traceModel: TraceModel, sha1: string): Headers | undefined {
+function downloadHeaders(searchParams: URLSearchParams): Headers | undefined {
-  const attachment = traceModel.attachmentForSha1(sha1);
+  const name = searchParams.get('dn');
-  if (!attachment)
+  const contentType = searchParams.get('dct');
+  if (!name)
     return;
   const headers = new Headers();
-  headers.set('Content-Disposition', `attachment; filename="attachment"; filename*=UTF-8''${encodeURIComponent(attachment.name)}`);
+  headers.set('Content-Disposition', `attachment; filename="attachment"; filename*=UTF-8''${encodeURIComponent(name)}`);
-  if (attachment.contentType)
+  if (contentType)
-    headers.set('Content-Type', attachment.contentType);
+    headers.set('Content-Type', contentType);
   return headers;
 }
 

packages/trace-viewer/src/traceModel.ts

@@ -14,7 +14,6 @@
  * limitations under the License.
  */
 
-import type * as trace from '@trace/trace';
 import { parseClientSideCallMetadata } from '../../../packages/playwright-core/src/utils/isomorphic/traceUtils';
 import type { ContextEntry } from './entries';
 import { createEmptyContext } from './entries';
@@ -34,7 +33,6 @@ export class TraceModel {
   contextEntries: ContextEntry[] = [];
   private _snapshotStorage: SnapshotStorage | undefined;
   private _backend!: TraceModelBackend;
-  private _attachments = new Map<string, trace.AfterActionTraceEventAttachment>();
   private _resourceToContentType = new Map<string, string>();
 
   constructor() {
@@ -64,7 +62,7 @@ export class TraceModel {
       const contextEntry = createEmptyContext();
       contextEntry.traceUrl = backend.traceURL();
       contextEntry.hasSource = hasSource;
-      const modernizer = new TraceModernizer(contextEntry, this._snapshotStorage, this._attachments);
+      const modernizer = new TraceModernizer(contextEntry, this._snapshotStorage);
 
       const trace = await this._backend.readText(ordinal + '.trace') || '';
       modernizer.appendTrace(trace);
@@ -121,10 +119,6 @@ export class TraceModel {
     return new Blob([blob], { type: this._resourceToContentType.get(sha1) || 'application/octet-stream' });
   }
 
-  attachmentForSha1(sha1: string): trace.AfterActionTraceEventAttachment | undefined {
-    return this._attachments.get(sha1);
-  }
-
   storage(): SnapshotStorage {
     return this._snapshotStorage!;
   }

packages/trace-viewer/src/traceModernizer.ts

@@ -34,17 +34,15 @@ const latestVersion: trace.VERSION = 7;
 export class TraceModernizer {
   private _contextEntry: ContextEntry;
   private _snapshotStorage: SnapshotStorage;
-  private _attachments: Map<string, trace.AfterActionTraceEventAttachment>;
   private _actionMap = new Map<string, ActionEntry>();
   private _version: number | undefined;
   private _pageEntries = new Map<string, PageEntry>();
   private _jsHandles = new Map<string, { preview: string }>();
   private _consoleObjects = new Map<string, { type: string, text: string, location: { url: string, lineNumber: number, columnNumber: number }, args?: { preview: string, value: string }[] }>();
 
-  constructor(contextEntry: ContextEntry, snapshotStorage: SnapshotStorage, attachments: Map<string, trace.AfterActionTraceEventAttachment>) {
+  constructor(contextEntry: ContextEntry, snapshotStorage: SnapshotStorage) {
     this._contextEntry = contextEntry;
     this._snapshotStorage = snapshotStorage;
-    this._attachments = attachments;
   }
 
   appendTrace(trace: string) {
@@ -129,8 +127,6 @@ export class TraceModernizer {
         existing!.attachments = event.attachments;
         if (event.point)
           existing!.point = event.point;
-        for (const attachment of event.attachments?.filter(a => a.sha1) || [])
-          this._attachments.set(attachment.sha1!, attachment);
         break;
       }
       case 'action': {

packages/trace-viewer/src/ui/attachmentsTab.tsx

@@ -50,7 +50,7 @@ const ExpandableAttachment: React.FunctionComponent<ExpandableAttachmentProps> =
   }, [expanded, attachmentText, placeholder, attachment]);
 
   const title = <span style={{ marginLeft: 5 }}>
-    {attachment.name} <a style={{ marginLeft: 5 }} href={attachmentURL(attachment) + '&download'}>download</a>
+    {attachment.name} <a style={{ marginLeft: 5 }} href={downloadURL(attachment)}>download</a>
   </span>;
 
   if (!isTextAttachment)
@@ -111,9 +111,9 @@ export const AttachmentsTab: React.FunctionComponent<{
         {expected && actual && <div className='attachments-section'>Image diff</div>}
         {expected && actual && <ImageDiffView noTargetBlank={true} diff={{
           name: 'Image diff',
-          expected: { attachment: { ...expected, path: attachmentURL(expected) + '&download' }, title: 'Expected' },
+          expected: { attachment: { ...expected, path: downloadURL(expected) }, title: 'Expected' },
-          actual: { attachment: { ...actual, path: attachmentURL(actual) + '&download' } },
+          actual: { attachment: { ...actual, path: downloadURL(actual) } },
-          diff: diff ? { attachment: { ...diff, path: attachmentURL(diff) + '&download' } } : undefined,
+          diff: diff ? { attachment: { ...diff, path: downloadURL(diff) } } : undefined,
         }} />}
       </>;
     })}
@@ -134,8 +134,19 @@ export const AttachmentsTab: React.FunctionComponent<{
   </div>;
 };
 
-function attachmentURL(attachment: Attachment) {
+function attachmentURL(attachment: Attachment, queryParams: Record<string, string> = {}) {
-  if (attachment.sha1)
+  const params = new URLSearchParams(queryParams);
-    return 'sha1/' + attachment.sha1 + '?trace=' + encodeURIComponent(attachment.traceUrl);
+  if (attachment.sha1) {
-  return 'file?path=' + encodeURIComponent(attachment.path!);
+    params.set('trace', attachment.traceUrl);
+    return 'sha1/' + attachment.sha1 + '?' + params.toString();
+  }
+  params.set('path', attachment.path!);
+  return 'file?' + params.toString();
+}
+
+function downloadURL(attachment: Attachment) {
+  const params = { dn: attachment.name } as Record<string, string>;
+  if (attachment.contentType)
+    params.dct = attachment.contentType;
+  return attachmentURL(attachment, params);
 }

tests/playwright-test/ui-mode-test-attachments.spec.ts

@@ -23,7 +23,10 @@ test('should contain text attachment', async ({ runUITest }) => {
     'a.test.ts': `
       import { test } from '@playwright/test';
       test('attach test', async () => {
+        // Attach two files with the same content and different names,
+        // to make sure each is downloaded with an intended name.
         await test.info().attach('file attachment', { path: __filename });
+        await test.info().attach('file attachment 2', { path: __filename });
         await test.info().attach('text attachment', { body: 'hi tester!', contentType: 'text/plain' });
       });
     `,
@@ -35,14 +38,24 @@ test('should contain text attachment', async ({ runUITest }) => {
 
   await page.locator('.tab-attachments').getByText('text attachment').click();
   await expect(page.locator('.tab-attachments')).toContainText('hi tester!');
-  await page.locator('.tab-attachments').getByText('file attachment').click();
+  await page.locator('.tab-attachments').getByText('file attachment').first().click();
   await expect(page.locator('.tab-attachments')).not.toContainText('attach test');
 
+  {
     const downloadPromise = page.waitForEvent('download');
     await page.getByRole('link', { name: 'download' }).first().click();
     const download = await downloadPromise;
     expect(download.suggestedFilename()).toBe('file attachment');
     expect((await readAllFromStream(await download.createReadStream())).toString()).toContain('attach test');
+  }
+
+  {
+    const downloadPromise = page.waitForEvent('download');
+    await page.getByRole('link', { name: 'download' }).nth(1).click();
+    const download = await downloadPromise;
+    expect(download.suggestedFilename()).toBe('file attachment 2');
+    expect((await readAllFromStream(await download.createReadStream())).toString()).toContain('attach test');
+  }
 });
 
 test('should contain binary attachment', async ({ runUITest }) => {