feat(plugin-adblocker): Add cooperative intercept mode support #592

FdezRomero · 2021-11-23T19:48:17Z

Adds support for the new Cooperative Intercept Mode available in Puppeteer v11+ and defining a custom interceptResolutionPriority for the following plugins:

puppeteer-extra-plugin-block-resources
puppeteer-extra-plugin-block-adblocker

This change checks that the required interception functions are available, so it's backwards compatible with the Legacy Mode and previous Puppeteer versions.

The @cliqz/adblocker-puppeteer dependency already included support for Cooperative Mode in v1.23.0.

Fixes #364, #156

packages/puppeteer-extra-plugin-adblocker/src/index.ts

packages/puppeteer-extra-plugin-block-resources/index.js

FdezRomero · 2021-11-23T21:57:30Z

@benallfree I implemented your suggestions and fixes. I also included a TypeScript definition that I was using on my client app 😉 Let me know if you find something else.

benallfree · 2021-11-23T22:11:09Z

packages/puppeteer-extra-plugin-block-resources/index.js

+      currentPriority
+    })
+
+    if (alreadyHandled) return


I think I missed this the first time. Can you walk me through the intent? In cooperative mode, the Puppeteer core will run all handlers and resolve the intercept to the one with the highest priority. Unless you have a special case, your handler should simply do its computation and respond at whatever priority it is set to. If a different response at a higher priority takes place, it will prevail and yours will “lose”. If yours is the highest priority response, it will “win”

@benallfree The problem is that if you use a plugin (or any other interceptor) that hasn't been migrated to Cooperative Mode and still uses Legacy Mode, the request is handled right away.

So if we call to continue/abort/respond etc. then we get the Request is already handled! error again (I just tried it again to verify). That's why we need this condition here, because we can't asume all plugins are opting-in to the new mode.

Oh that’s very smart, I think I’ll suggest this pattern in the official docs! Well done.

benallfree · 2021-11-24T13:24:11Z

packages/puppeteer-extra-plugin-block-resources/index.js

+      currentPriority
+    })
+
+    if (alreadyHandled) return


Oh that’s very smart, I think I’ll suggest this pattern in the official docs! Well done.

FdezRomero · 2021-11-24T17:57:10Z

@benallfree Glad to hear that! It seems that this PR needs a second approval. Do you know who can help with that? Thanks!

benallfree · 2021-11-24T20:48:14Z

@FdezRomero I'm not a maintainer of this package so I'm not sure how to help with a 2nd approval.

By the way, I just submitted puppeteer/puppeteer#7796 based on your work here. It fixes that spelling mistake and officially exposes isInterceptResolutionHandled() logic. You might consider waiting on this merge until we know whether the Puppeteer team will accept my PR.

benallfree · 2021-11-25T10:58:36Z

@FdezRomero Small note, cooperative support came in 10.2 https://github.com/puppeteer/puppeteer/blob/v11.0.0/CHANGELOG.md#1020-2021-08-04

FdezRomero · 2021-11-25T15:00:35Z

@benallfree Ah nice! I changed the comment, and will wait for the API improvements in Puppeteer 👍🏻

We will have to update the API in @cliqz/adblocker-puppeteer as well so it doesn't break.

benallfree · 2021-11-25T16:44:58Z

@FdezRomero What will break? #7796 does not introduce breaking changes, or it’s not supposed to :)

FdezRomero · 2021-11-26T16:27:52Z

You're right @benallfree, @cliqz/adblocker-puppeteer isn't using the interceptResolution() method to check if the request has been already handled or not, so it won't be affected by the rename to interceptResolutionState() 👍🏻

I moved this PR back to draft and will apply the changes once puppeteer/puppeteer#7796 ships. Hopefully it gets reviewed soon!.

benallfree · 2021-12-07T13:38:37Z

@FdezRomero puppeteer/puppeteer#7796 has been accepted by the Puppeteer team 👍

FdezRomero · 2021-12-07T16:39:27Z

@benallfree Great! I'll update the PR with the new API 👍🏻
Any idea when a new version might be released?

benallfree · 2021-12-07T16:58:55Z

@FdezRomero No idea 😅

…tate

FdezRomero · 2021-12-12T19:58:58Z

Hi @benallfree, I have updated adblocker-puppeteer to the latest version with the fixes and upgraded the code to your newer Puppeteer API, which also made the code easier to read. Would you mind giving it another look? Thanks! 🙏🏻

benallfree

lgtm w optional suggestions 👍

benallfree · 2021-12-12T21:58:05Z

packages/puppeteer-extra-plugin-adblocker/readme.md

-puppeteer.use(AdblockerPlugin())
+puppeteer.use(AdblockerPlugin({
+  // Optionally enable Cooperative Mode for several request interceptors
+  interceptResolutionPriority: 0


Safer as DEFAULT_INTERCEPT_RESOLUTION_PRIORITY , but I'm not sure if they export it. If not, that's a fix for me :D

Yes, it's exported. I changed it in all examples 👍🏻

benallfree · 2021-12-12T21:58:55Z

packages/puppeteer-extra-plugin-block-resources/example.js

-const blockResourcesPlugin = require('puppeteer-extra-plugin-block-resources')()
+const blockResourcesPlugin = require('puppeteer-extra-plugin-block-resources')({
+  // Optionally enable Cooperative Mode for several request interceptors
+  interceptResolutionPriority: 0


benallfree · 2021-12-12T22:01:29Z

packages/puppeteer-extra-plugin-block-resources/index.js

+    if (alreadyHandled) return
+
+    if (shouldBlock) {
+      const abortArgs = request.continueRequestOverrides


Are you using continueRequestOverrides purely for version detection? If so, it might make more sense to use abortErrorReason since this is an abort() call.

I didn't know about this method, thanks! I changed it for the feature detection 👍🏻

benallfree · 2021-12-12T22:02:21Z

packages/puppeteer-extra-plugin-adblocker/src/index.ts

+   * Sets the request interception priority on the `PuppeteerBlocker` instance.
+   */
+  private setRequestInterceptionPriority(): void {
+    this.blocker?.setRequestInterceptionPriority(this.opts.interceptResolutionPriority)


Not familiar enough with the lib to know if this is correct, it seems to be passing it down a layer?

@benallfree Yes exactly, it references the instance of adblocker-puppeteer that is used internally, which is the one that really intercepts and handles the requests, and this pupeteer-extra plugin acts as a wrapper: https://github.com/ghostery/adblocker/blob/master/packages/adblocker-puppeteer/adblocker.ts#L282-L283

FdezRomero · 2021-12-13T17:33:05Z

I think it's good to go! Let's see if it can get a bit of love from the maintainers 🤞🏻😄 @berstend

FdezRomero · 2022-01-13T17:55:03Z

Happy new year @berstend! 🎉

How can we get a second review to get this merged? We already made the necessary changes upstream in Puppeteer and Adblocker to support Cooperative Intercept mode in puppeteer-extra 😉

Thanks!

deldrid1 · 2022-02-18T21:22:04Z

@berstend - Any chance this can be reviewed? This is a critical feature I need!

benallfree · 2022-02-18T22:11:35Z

@deldrid1 @berstend You both might consider running your own fork for a while, it seems the maintainer is busy on other things.

berstend · 2022-06-20T14:43:46Z

Thanks! LGTM

berstend · 2022-06-20T15:51:39Z

Successfully published:
 - puppeteer-extra-plugin-adblocker@2.13.0
 - puppeteer-extra-plugin-block-resources@2.4.0

FdezRomero added 2 commits November 23, 2021 20:40

feat(plugin-block-resources): add interception resolution mode

79e40fa

feat(plugin-adblocker): add interception resolution mode

b3ad770

github-actions bot added the plugin: puppeteer-extra PuppeteerExtra Plugin related label Nov 23, 2021

FdezRomero mentioned this pull request Nov 23, 2021

Solving request interception chain #364

Closed

benallfree suggested changes Nov 23, 2021

View reviewed changes

packages/puppeteer-extra-plugin-adblocker/src/index.ts Outdated Show resolved Hide resolved

packages/puppeteer-extra-plugin-block-resources/index.js Outdated Show resolved Hide resolved

benallfree suggested changes Nov 23, 2021

View reviewed changes

packages/puppeteer-extra-plugin-block-resources/index.js Outdated Show resolved Hide resolved

FdezRomero added 3 commits November 23, 2021 22:51

fix(plugin-block-resources): disable by default, backwards compatibility

eaf0cd0

feat(plugin-block-resources): add typescript definitions

4faf92f

fix(plugin-adblocker): disable by default, backwards compatibility

6e99115

test(plugin-block-resources): fix test

c4f5bee

benallfree suggested changes Nov 23, 2021

View reviewed changes

style: fix identation

961e08b

benallfree approved these changes Nov 24, 2021

View reviewed changes

benallfree mentioned this pull request Nov 24, 2021

Feat: Expose HTTPRequest intercept resolution state and clarify docs puppeteer/puppeteer#7795

Closed

benallfree mentioned this pull request Nov 24, 2021

Feat: Expose HTTPRequest intercept resolution state and clarify docs puppeteer/puppeteer#7796

Merged

fix(plugin-block-resources): fix comment

5e9fb27

FdezRomero marked this pull request as draft November 25, 2021 15:00

fix(plugin-block-resources): use new API in puppeteer v13

f43833f

FdezRomero mentioned this pull request Dec 10, 2021

Update Puppeteer Cooperative Intercept Mode ghostery/adblocker#2343

Merged

chore: run tests with puppeteer v10.2.0

262980e

github-actions bot added the package: core Affecting a core package label Dec 10, 2021

FdezRomero marked this pull request as ready for review December 10, 2021 19:45

FdezRomero added 2 commits December 12, 2021 20:13

chore: update adblocker-puppeteer to v1.23.2

53ff766

fix(plugin-block-resources): remove not needed intercept resolution s…

6075098

…tate

benallfree approved these changes Dec 12, 2021

View reviewed changes

fix: apply PR suggestions

784e1ad

benallfree mentioned this pull request Feb 3, 2022

Request interception hanging forever puppeteer/puppeteer#7475

Closed

benallfree approved these changes Feb 3, 2022

View reviewed changes

deldrid1 approved these changes Feb 18, 2022

View reviewed changes

DustinJSilk mentioned this pull request May 13, 2022

UnhandledPromiseRejectionWarning: Error: Request is already handled! puppeteer/puppeteer#5334

Closed

berstend changed the title ~~feat: add cooperative intercept mode support~~ feat(plugin-adblocker): Add cooperative intercept mode support Jun 20, 2022

berstend merged commit c0daeda into berstend:master Jun 20, 2022

berstend mentioned this pull request Jun 20, 2022

[Bug] Using adblocker and block-resources plugins together causes an error #611

Closed

berstend mentioned this pull request Jul 1, 2022

[Bug] adblocker doesn't check if request is already intercepted #600

Closed

berstend mentioned this pull request Jul 8, 2022

[Bug] BlockResourcesPlugin does not work alongside with AdblockerPlugin. #585

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(plugin-adblocker): Add cooperative intercept mode support #592

feat(plugin-adblocker): Add cooperative intercept mode support #592

FdezRomero commented Nov 23, 2021 •

edited

FdezRomero commented Nov 23, 2021

benallfree Nov 23, 2021

FdezRomero Nov 24, 2021 •

edited

benallfree Nov 24, 2021

benallfree Nov 24, 2021

FdezRomero commented Nov 24, 2021

benallfree commented Nov 24, 2021 •

edited

benallfree commented Nov 25, 2021

FdezRomero commented Nov 25, 2021

benallfree commented Nov 25, 2021

FdezRomero commented Nov 26, 2021 •

edited

benallfree commented Dec 7, 2021

FdezRomero commented Dec 7, 2021

benallfree commented Dec 7, 2021

FdezRomero commented Dec 12, 2021

benallfree left a comment

benallfree Dec 12, 2021

FdezRomero Dec 13, 2021

benallfree Dec 12, 2021

benallfree Dec 12, 2021

FdezRomero Dec 13, 2021

benallfree Dec 12, 2021

FdezRomero Dec 13, 2021

FdezRomero commented Dec 13, 2021

FdezRomero commented Jan 13, 2022

deldrid1 commented Feb 18, 2022

benallfree commented Feb 18, 2022

berstend commented Jun 20, 2022

berstend commented Jun 20, 2022

feat(plugin-adblocker): Add cooperative intercept mode support #592

feat(plugin-adblocker): Add cooperative intercept mode support #592

Conversation

FdezRomero commented Nov 23, 2021 • edited

FdezRomero commented Nov 23, 2021

Choose a reason for hiding this comment

FdezRomero Nov 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FdezRomero commented Nov 24, 2021

benallfree commented Nov 24, 2021 • edited

benallfree commented Nov 25, 2021

FdezRomero commented Nov 25, 2021

benallfree commented Nov 25, 2021

FdezRomero commented Nov 26, 2021 • edited

benallfree commented Dec 7, 2021

FdezRomero commented Dec 7, 2021

benallfree commented Dec 7, 2021

FdezRomero commented Dec 12, 2021

benallfree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FdezRomero commented Dec 13, 2021

FdezRomero commented Jan 13, 2022

deldrid1 commented Feb 18, 2022

benallfree commented Feb 18, 2022

berstend commented Jun 20, 2022

berstend commented Jun 20, 2022

FdezRomero commented Nov 23, 2021 •

edited

FdezRomero Nov 24, 2021 •

edited

benallfree commented Nov 24, 2021 •

edited

FdezRomero commented Nov 26, 2021 •

edited