Skip to content

Commit

Permalink
[LayoutShift] Disable iframe root causes by default
Browse files Browse the repository at this point in the history 
The iframe root cause check casts a very wide net for events that
could have caused an iframe to be added. We are eventually going to drop
the CDP requirement for root causes, so it isn't worth it to make the
process more efficient.

Follow-up to:
https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/5384002

Lighthouse issue:
GoogleChrome/lighthouse#15869

Bug: 331429737
Change-Id: I68d51ac6b3ed8910ef0b032f885608baf04594b9
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/5421436
Reviewed-by: Paul Irish <paulirish@chromium.org>
Commit-Queue: Adam Raine <asraine@chromium.org>
  • Loading branch information
Adam Raine authored and Devtools-frontend LUCI CQ committed Apr 5, 2024
1 parent 69a3bdf commit 7fdc549
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 28 deletions.
87 changes: 61 additions & 26 deletions front_end/models/trace/root-causes/LayoutShift.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import {
import {getBaseTraceParseModelData} from '../../../testing/TraceHelpers.js';
import * as TraceEngine from '../trace.js';

import * as RootCauses from './RootCauses.js';

type TraceParseData = TraceEngine.Handlers.Types.TraceParseData;
type TraceParseDataMutable = TraceEngine.Handlers.Types.TraceParseDataMutable;

Expand Down Expand Up @@ -47,7 +49,7 @@ describeWithMockConnection('LayoutShift root causes', () => {
* required.
*/
describe('assigns root causes to layout shifts', () => {
let rootCausesEngine: TraceEngine.RootCauses.RootCauses.RootCauses;
let layoutShifts: RootCauses.LayoutShiftRootCauses;
let prePaintEvents: TraceEngine.Types.TraceEvents.TraceEventPrePaint[];
let resizeEvents: TraceEngine.Types.TraceEvents.TraceEventLayoutInvalidationTracking[];
let injectedIframeEvents: TraceEngine.Types.TraceEvents.TraceEventLayoutInvalidationTracking[];
Expand All @@ -60,6 +62,7 @@ describeWithMockConnection('LayoutShift root causes', () => {
let iframesNodeIds: number[];
let shifts: TraceEngine.Types.TraceEvents.SyntheticLayoutShift[];
let matchedStylesMock: Omit<Protocol.CSS.GetMatchedStylesForNodeResponse, 'getError'>;
let protocolInterface: RootCauses.RootCauseProtocolInterface;
let computedStylesMock: Protocol.CSS.CSSComputedStyleProperty[];
let fontFaceMock: Protocol.CSS.FontFace;
const fontSource = 'mock-source.woff';
Expand Down Expand Up @@ -109,6 +112,7 @@ describeWithMockConnection('LayoutShift root causes', () => {
data: {
nodeId: i + 1 as Protocol.DOM.BackendNodeId,
reason: TraceEngine.Types.TraceEvents.LayoutInvalidationReason.SIZE_CHANGED,
nodeName: 'IMG',
frame: 'frame-id-123',
},
};
Expand All @@ -118,6 +122,7 @@ describeWithMockConnection('LayoutShift root causes', () => {
data: {
nodeId: i + 11 as Protocol.DOM.BackendNodeId,
reason: TraceEngine.Types.TraceEvents.LayoutInvalidationReason.ADDED_TO_LAYOUT,
nodeName: 'IFRAME',
frame: 'frame-id-123',
},
};
Expand All @@ -127,6 +132,7 @@ describeWithMockConnection('LayoutShift root causes', () => {
data: {
nodeId: i + 21 as Protocol.DOM.BackendNodeId,
reason: TraceEngine.Types.TraceEvents.LayoutInvalidationReason.FONTS_CHANGED,
nodeName: 'DIV',
frame: 'frame-id-123',
},
};
Expand All @@ -136,6 +142,7 @@ describeWithMockConnection('LayoutShift root causes', () => {
data: {
nodeId: i + 31 as Protocol.DOM.BackendNodeId,
reason: TraceEngine.Types.TraceEvents.LayoutInvalidationReason.UNKNOWN,
nodeName: 'DIV',
frame: 'frame-id-123',
},
};
Expand All @@ -158,14 +165,12 @@ describeWithMockConnection('LayoutShift root causes', () => {
for (let i = 0 as Protocol.DOM.BackendNodeId; i < layoutInvalidationEvents.length; i++) {
const backendNodeId = layoutInvalidationEvents[i].args.data.nodeId;
const nodeId = i as unknown as Protocol.DOM.NodeId;
const nodeName = layoutInvalidationEvents[i].args.data.nodeName || 'DIV';
const fakeNode = {
backendNodeId,
nodeId,
localName: 'img',
nodeName: layoutInvalidationEvents[i].args.data.reason ===
TraceEngine.Types.TraceEvents.LayoutInvalidationReason.ADDED_TO_LAYOUT ?
'IFRAME' :
'IMG',
localName: nodeName.toLowerCase(),
nodeName,
attributes: [],
nodeType: Node.ELEMENT_NODE,
} as unknown as Protocol.DOM.Node;
Expand Down Expand Up @@ -196,7 +201,7 @@ describeWithMockConnection('LayoutShift root causes', () => {
computedStylesMock = [];
matchedStylesMock = {};

rootCausesEngine = new TraceEngine.RootCauses.RootCauses.RootCauses({
protocolInterface = {
getInitiatorForRequest(_: string): Protocol.Network.Initiator |
null {
return null;
Expand Down Expand Up @@ -234,14 +239,32 @@ describeWithMockConnection('LayoutShift root causes', () => {
}
return;
},
};

layoutShifts = new RootCauses.LayoutShiftRootCauses(protocolInterface, {enableIframeRootCauses: true});
});

it('uses cached node details', async () => {
// Use duplicate node ids for invalidation events that use `getNode`
resizeEvents.forEach(e => {
e.args.data.nodeId = 1 as Protocol.DOM.BackendNodeId;
});
injectedIframeEvents.forEach(e => {
e.args.data.nodeId = 11 as Protocol.DOM.BackendNodeId;
});

const getNodeSpy = sinon.spy(protocolInterface, 'getNode');

const rootCauses = await Promise.all(shifts.map(shift => layoutShifts.rootCausesForEvent(model, shift)));
assertArrayHasNoNulls(rootCauses);

assert.strictEqual(getNodeSpy.callCount, 2);
});

describe('Unsized media', () => {
it('marks unsized media node in LayoutInvalidation events as a potential root cause to layout shifts correctly',
async () => {
const rootCauses =
await Promise.all(shifts.map(shift => rootCausesEngine.layoutShifts.rootCausesForEvent(model, shift)));
const rootCauses = await Promise.all(shifts.map(shift => layoutShifts.rootCausesForEvent(model, shift)));
assertArrayHasNoNulls(rootCauses);

const shiftCausesNodeIds = rootCauses.map(cause => {
Expand Down Expand Up @@ -272,7 +295,7 @@ describeWithMockConnection('LayoutShift root causes', () => {
inlineStyle: createMockStyle([{name: 'height', value: '20px'}]),
matchedCSSRules: createMockMatchedRules([{name: 'height', value: '10px'}]),
};
const rootCause = await rootCausesEngine.layoutShifts.rootCausesForEvent(model, shifts[0]);
const rootCause = await layoutShifts.rootCausesForEvent(model, shifts[0]);

const authoredDimensions = rootCause?.unsizedMedia[0].authoredDimensions;
if (!authoredDimensions) {
Expand All @@ -293,7 +316,7 @@ describeWithMockConnection('LayoutShift root causes', () => {
matchedCSSRules: createMockMatchedRules([{name: 'height', value: '30px'}]),
};

const rootCause = await rootCausesEngine.layoutShifts.rootCausesForEvent(model, shifts[1]);
const rootCause = await layoutShifts.rootCausesForEvent(model, shifts[1]);
const authoredDimensions = rootCause?.unsizedMedia[0].authoredDimensions;
if (!authoredDimensions) {
assert.fail('Expected defined authored dimensions');
Expand All @@ -311,7 +334,7 @@ describeWithMockConnection('LayoutShift root causes', () => {
{name: 'width', value: width},
];

const rootCause = await rootCausesEngine.layoutShifts.rootCausesForEvent(model, shifts[1]);
const rootCause = await layoutShifts.rootCausesForEvent(model, shifts[1]);
const computedDimensions = rootCause?.unsizedMedia[0].computedDimensions;
if (!computedDimensions) {
assert.fail('Expected defined computed dimensions');
Expand All @@ -324,7 +347,7 @@ describeWithMockConnection('LayoutShift root causes', () => {

async function assertAmountOfBlamedLayoutInvalidations(amount: number) {
const allShiftsRootCauses =
await Promise.all(shifts.map(shift => rootCausesEngine.layoutShifts.rootCausesForEvent(model, shift)));
await Promise.all(shifts.map(shift => layoutShifts.rootCausesForEvent(model, shift)));

const nodesFromLayoutInvalidations = new Set<number>();
for (const currentShiftRootCauses of allShiftsRootCauses) {
Expand Down Expand Up @@ -405,16 +428,15 @@ describeWithMockConnection('LayoutShift root causes', () => {
modelMut.LayoutShifts.clusters = clusters;

assert.doesNotThrow(async () => {
await Promise.all(shifts.map(shift => rootCausesEngine.layoutShifts.rootCausesForEvent(model, shift)));
await Promise.all(shifts.map(shift => layoutShifts.rootCausesForEvent(model, shift)));
});
});
});

describe('Injected iframes', () => {
it('marks injected iframes in LayoutInvalidation events as a potential root cause to layout shifts correctly',
async () => {
const rootCauses =
await Promise.all(shifts.map(shift => rootCausesEngine.layoutShifts.rootCausesForEvent(model, shift)));
const rootCauses = await Promise.all(shifts.map(shift => layoutShifts.rootCausesForEvent(model, shift)));
assertArrayHasNoNulls(rootCauses);
const shiftCausesNodeIds = rootCauses.map(cause => {
return cause.iframes.map(node => Number(node.iframe.backendNodeId));
Expand All @@ -427,6 +449,24 @@ describeWithMockConnection('LayoutShift root causes', () => {
assert.strictEqual(shiftCausesNodeIds[4].length, 1);
assert.strictEqual(shiftCausesNodeIds[4][0], iframesNodeIds[1]);
});

it('ignores injected iframes if disabled', async () => {
layoutShifts = new RootCauses.LayoutShiftRootCauses(protocolInterface, {enableIframeRootCauses: false});
const rootCauses = await Promise.all(shifts.map(shift => layoutShifts.rootCausesForEvent(model, shift)));
assertArrayHasNoNulls(rootCauses);
assert(rootCauses.every(cause => cause.iframes.length === 0), 'contained iframe root causes');
});

it('ignores events that could not add or resize an iframe', async () => {
injectedIframeEvents.forEach(e => {
e.args.data.nodeName = 'DIV';
e.args.data.reason = TraceEngine.Types.TraceEvents.LayoutInvalidationReason.SIZE_CHANGED;
});

const rootCauses = await Promise.all(shifts.map(shift => layoutShifts.rootCausesForEvent(model, shift)));
assertArrayHasNoNulls(rootCauses);
assert(rootCauses.every(cause => cause.iframes.length === 0), 'contained iframe root causes');
});
});

describe('Font changes', () => {
Expand Down Expand Up @@ -459,8 +499,7 @@ describeWithMockConnection('LayoutShift root causes', () => {
async () => {
modelMut.NetworkRequests.byTime = fontRequests;

const rootCauses =
await Promise.all(shifts.map(shift => rootCausesEngine.layoutShifts.rootCausesForEvent(model, shift)));
const rootCauses = await Promise.all(shifts.map(shift => layoutShifts.rootCausesForEvent(model, shift)));
assertArrayHasNoNulls(rootCauses);
const shiftCausesNodeIds = rootCauses.map(cause => {
return cause.fontChanges;
Expand Down Expand Up @@ -489,8 +528,7 @@ describeWithMockConnection('LayoutShift root causes', () => {
}] as unknown as TraceEngine.Types.TraceEvents.SyntheticNetworkRequest[];
modelMut.NetworkRequests.byTime = optionalFontRequests;
fontFaceMock = {fontFamily: 'Roboto', src: fontSource, fontDisplay: 'optional'} as Protocol.CSS.FontFace;
const rootCauses =
await Promise.all(shifts.map(shift => rootCausesEngine.layoutShifts.rootCausesForEvent(model, shift)));
const rootCauses = await Promise.all(shifts.map(shift => layoutShifts.rootCausesForEvent(model, shift)));
assertArrayHasNoNulls(rootCauses);
const shiftCausesNodeIds = rootCauses.map(cause => {
return cause.fontChanges;
Expand All @@ -516,8 +554,7 @@ describeWithMockConnection('LayoutShift root causes', () => {
}] as unknown as TraceEngine.Types.TraceEvents.SyntheticNetworkRequest[];
modelMut.NetworkRequests.byTime = optionalFontRequests;
fontFaceMock = {fontFamily: 'Roboto', src: fontSource, fontDisplay: 'swap'} as Protocol.CSS.FontFace;
const rootCauses =
await Promise.all(shifts.map(shift => rootCausesEngine.layoutShifts.rootCausesForEvent(model, shift)));
const rootCauses = await Promise.all(shifts.map(shift => layoutShifts.rootCausesForEvent(model, shift)));
assertArrayHasNoNulls(rootCauses);
const shiftCausesNodeIds = rootCauses.map(cause => {
return cause.fontChanges;
Expand Down Expand Up @@ -561,8 +598,7 @@ describeWithMockConnection('LayoutShift root causes', () => {
async () => {
modelMut.NetworkRequests.byTime = RenderBlockingRequest;

const rootCauses =
await Promise.all(shifts.map(shift => rootCausesEngine.layoutShifts.rootCausesForEvent(model, shift)));
const rootCauses = await Promise.all(shifts.map(shift => layoutShifts.rootCausesForEvent(model, shift)));
assertArrayHasNoNulls(rootCauses);
const shiftCausesNodeIds = rootCauses.map(cause => {
return cause.renderBlockingRequests;
Expand Down Expand Up @@ -617,8 +653,7 @@ describeWithMockConnection('LayoutShift root causes', () => {
} as TraceEngine.Types.TraceEvents.TraceEventData);

// Verify the Layout initiator's stack trace is added to the last shift.
const rootCauses =
await Promise.all(shifts.map(shift => rootCausesEngine.layoutShifts.rootCausesForEvent(model, shift)));
const rootCauses = await Promise.all(shifts.map(shift => layoutShifts.rootCausesForEvent(model, shift)));
assertArrayHasNoNulls(rootCauses);
const rootCauseStackTraces = rootCauses.map(cause => {
return cause.scriptStackTrace;
Expand Down
15 changes: 13 additions & 2 deletions front_end/models/trace/root-causes/LayoutShift.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,20 @@ function networkRequestIsRenderBlockingInFrame(
return isRenderBlocking && event.args.data.frame === frameId;
}

interface Options {
/** Checking iframe root causes can be an expensive operation, so it is disabled by default. */
enableIframeRootCauses?: boolean;
}

export class LayoutShiftRootCauses {
#protocolInterface: RootCauseProtocolInterface;
#rootCauseCacheMap = new Map<Types.TraceEvents.TraceEventLayoutShift, LayoutShiftRootCausesData>();
#nodeDetailsCache = new Map<Protocol.DOM.NodeId, Protocol.DOM.Node|null>();
#iframeRootCausesEnabled: boolean;

constructor(protocolInterface: RootCauseProtocolInterface) {
constructor(protocolInterface: RootCauseProtocolInterface, options?: Options) {
this.#protocolInterface = protocolInterface;
this.#iframeRootCausesEnabled = options?.enableIframeRootCauses ?? false;
}

/**
Expand Down Expand Up @@ -323,7 +330,11 @@ export class LayoutShiftRootCauses {
async getIframeRootCause(
layoutInvalidation: Types.TraceEvents.TraceEventLayoutInvalidationTracking,
layoutInvalidationNodeId: Protocol.DOM.NodeId): Promise<InjectedIframe|null> {
if (layoutInvalidation.args.data.nodeName?.startsWith('IFRAME') &&
if (!this.#iframeRootCausesEnabled) {
return null;
}

if (!layoutInvalidation.args.data.nodeName?.startsWith('IFRAME') &&
layoutInvalidation.args.data.reason !== Types.TraceEvents.LayoutInvalidationReason.STYLE_CHANGED &&
layoutInvalidation.args.data.reason !== Types.TraceEvents.LayoutInvalidationReason.ADDED_TO_LAYOUT) {
return null;
Expand Down
2 changes: 2 additions & 0 deletions front_end/models/trace/root-causes/RootCauses.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,5 @@ export class RootCauses {
this.layoutShifts = new LayoutShiftRootCauses(protocolInterface);
}
}

export {LayoutShiftRootCauses} from './LayoutShift.js';

0 comments on commit 7fdc549

Please sign in to comment.