Request interception and caching compatibility

[Androbin]

See #2905

This change is fully backwards compatible as it defaults to the current behavior.

[Androbin]

This is not a perfect solution, as outlined in #2905 (comment). It should cover most people's use cases though, while we are waiting for Chromium changes to land.

[OrKoN]

Jan, could you PTAL since you looked into the request interception recently?

[jschfflr]

LGTM, thanks!

[jschfflr]

One thing though: Could you add a unit test to cover this use case?

[jschfflr]

Ah damn, I'm confused by this UI.
Could you add a test that covers this new behaviour?

[Androbin]

When we call Network.setCacheDisabled({cacheDisabled: false}), certain events are not fired, including Page.Events.Response. To get around this, the test is listening to the newly added Page.Events.RequestServedFromCache instead.

[jribbens]

Can I suggest that the documentation update should give the merest clue as to why you might or might not want caching to be disabled? If I'm understanding it correctly, the only problem is that if caching is not disabled, then requests served from the cache will not be intercepted - which may well not be a problem at all in many applications.

[starrify]

I'm under the impression that caching is purposely disabled when configuring request interception because otherwise unexpected issues would happen. This is also stated by the original author @aslushnikov of the relevant code in #1154 and #4260.

It is relatively easy to just configure request interception and having caching enabled at the same time. I have tried that some time ago and have got the similar results (rendering some pages never finish due to lack of the load event in page) as described in this comment: #5176 (comment) . (sorry I'm unable to share further details on my previous tests)

Unfortunately, this PR does not seem to resolve the earlier issue as it merely provides the means for users to keep caching enabled while doing request interception.

I have performed further tests, and it seems that Puppeteer at 8695759 (revision that has this PR merged) would fail to render many webpages, one example of which is https://github.com/, while having page.setRequestInterception(true, true);. I have created issue #7038 to share further details.

I'm afraid that this PR may need to be excluded from the next Puppeteer release unless properly fixed, as the current version merely reintroduced the issue that was previously got rid of in #1154 and #4260.

[Androbin]

@starrify I am fully aware that page.setRequestInterception(true, true); might misbehave for some use cases.
However there are people with use cases where this doesn't break and they really need this flag.
It's opt-in, so there is no harm in keeping the flag. We can however document it as experimental.

[starrify]

Right, I agree that people with specific use cases may be interested in this, though I much doubt the coverage as there are many real-world web pages that would break on this, according to my previous tests and what's mentioned here: #5176 (comment)

@Androbin: Could you help quickly verify whether it is true that Puppeteer at revision 8695759 (with this PR merged) would fail to render https://github.com/ ? (click me for a sample test script)

(async () => {
  const puppeteer = require('puppeteer');
  const browser = await puppeteer.launch({
    // Needed only when inside a Docker container
    args: ['--no-sandbox'],
  });
  const page = await browser.newPage();
  const enableCaching = process.env.TEST_ENABLE_CACHING == 'true';
  await page.setRequestInterception(true, enableCaching);
  page.on('request', (request) => {
    request.continue();
  });
  const url = process.env.TEST_URL || 'https://github.com/';
  try {
    await page.goto(url);
  } catch (err) {
    console.error(err);
  }
  await browser.close();
})();

Sorry but I'm against the very idea of have it documented as experimental, as I would like to see from the documents clear warnings that this feature is known to break the rendering of many real world web pages (if my other issue report doesn't get proved wrong).

[Androbin]

const { page, server } = getTestState();
await page.setRequestInterception(true, false);
page.on('request', (request) => request.continue());
await page.goto('https://github.com/', { waitUntil: 'load' });

PASSED

const { page, server } = getTestState();
await page.setRequestInterception(true, true);
page.on('request', (request) => request.continue());
await page.goto('https://github.com/', { waitUntil: 'domcontentloaded' });

PASSED

const { page, server } = getTestState();
await page.setRequestInterception(true, true);
page.on('request', (request) => request.continue());
await page.goto('https://github.com/', { waitUntil: 'load' });

FAILED

const { page, server } = getTestState();
await page.setRequestInterception(true, true);
page.on('request', (request) => {
  // alternative: request.resourceType() == "font"
  if (request.url().endsWith(".woff")) {
    request.abort();
  } else {
    request.continue();
  }
});
await page.goto('https://github.com/', { waitUntil: 'load' });

PASSED

[Androbin]

I would like to note that blocking requests for fonts and images is a common use case of the people who requested this flag.

[Androbin]

@starrify If you provide me with a list of sites that have this issue, I could compile a list of failing resources, we could identify what's common between them, and fix the issue.

[Androbin]

I have done some debugging and it looks like the events generated by Chromium are in a different order for fonts than for other resources like stylesheets.

Stylesheet request:

• onRequestWillBeSent
• onRequestPaused (calls request.continue())
• onRequest
• onResponseReceived
• onLoadingFinished

Font request (cache disabled):

• onRequestPaused
• onRequestWillBeSent (calls request.continue())
• onRequest
• onResponseReceived
• onLoadingFinished

Font request (cache enabled):

• onRequestPaused
• onRequestWillBeSent (calls request.continue())
• onRequest
• onRequestPaused (does not call request.continue())
• request pending indefinitely

Font request (cache enabled + fix):

• onRequestPaused
• onRequestWillBeSent (calls request.continue())
• onRequest
• onRequestPaused (calls request.continue())
• onRequest
• onResponseReceived
• onLoadingFinished

[jschfflr]

Could you also check a scenario with redirects and how they change the order of events?