1753939 - Event log.entryAdded should not report Console API calls for webextensions (webcompat)

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Several newly filed bugs for intermittent test failures of WebDriver BiDi tests indicate that log.entryAdded events are getting emitted for the webcompat webextension which basically should not happen. By default our code should only emit these events for anything happening on the web page in the child process.

This new behavior started on Saturday Feb 5th with the merge of the wpt downstream sync over on bug 1752292:

https://treeherder.mozilla.org/jobs?repo=mozilla-central&searchStr=wd1%2Cccov

There are actually two questions:

1. What changed in our code on mozilla-central to let the webcompat extension call console.debug() now. I had a look at the sync but because there are way too many changes I didn't find anything that would explain it. Our tests only use web pages as served locally from the web-platform.test domain so the webcompat extension shouldn't inject something. Dennis, are you aware of anything that might have changed? Happy to get a new bug filed if needed. Thanks!

2. How can we prevent reporting of these log events? As @jdescottes mentioned over on bug 1753907 we might want to exclude webextension contexts beside the parent contexts that we already exclude. He proposed the following code:

isExtensionContext (browsingContext) {
  if (browsingContext instanceof CanonicalBrowsingContext) {
    return browsingContext.currentWindowGlobal.documentPrincipal.isAddonOrExpandedAddonPrincipal;
  }
  return browsingContext.window.document.nodePrincipal.isAddonOrExpandedAddonPrincipal;
}

But I wonder if we should better have a single helper method for the content scope check that actually whitelists the browsing contexts that we actually want. Otherwise we might have a lot of individual checks (helpers) in the future.

Comment 1

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #0)

But I wonder if we should better have a single helper method for the content scope check that actually whitelists the browsing contexts that we actually want. Otherwise we might have a lot of individual checks (helpers) in the future.

I think it's important to keep this logic running both in content and parent processes. As soon as we will want to support chrome scope, we will have to loosen the JSWindowActor definition to potentially spawn everywhere, and the isBrowsingContextCompatible should be the method "that actually whitelists the browsing contexts that we actually want". Or I am missing something in your suggestion? How would it differ from the current helper beyond restricting it to run only in the content process?

Note that isExtensionContext is not meant to be exposed outside of the FrameContextUtils.jsm module, it should only be reused in isBrowsingContextCompatible.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

On bug 1753853 I can see the following log entry from the webcompat extension:

[task 2022-02-05T22:28:27.084Z] 22:28:27     INFO -  'text': 'Enabling content scripts for these URLs: '
[task 2022-02-05T22:28:27.084Z] 22:28:27     INFO -          '*://*.stackblitz.com/*,*://*.stackblitz.io/*',

Why do we enable these content scripts for wpt?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Julian Descottes [:jdescottes] from comment #1)

I think it's important to keep this logic running both in content and parent processes. As soon as we will want to support chrome scope, we will have to loosen the JSWindowActor definition to potentially spawn everywhere, and the isBrowsingContextCompatible should be the method "that actually whitelists the browsing contexts that we actually want". Or I am missing something in your suggestion? How would it differ from the current helper beyond restricting it to run only in the content process?

Maybe there will be other checks that we are not aware of yet and would have to run too to ensure that we have a browsing context from a page. So I wonder if there is a way to just ignore every unrelated browsing context by doing just a single check. Maybe there is also a principal (or whatever else) available that could be checked instead? Means I'm more in favor of a whitelist and not a blocklist for content scope.

Comment 4

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #3)

(In reply to Julian Descottes [:jdescottes] from comment #1)

I think it's important to keep this logic running both in content and parent processes. As soon as we will want to support chrome scope, we will have to loosen the JSWindowActor definition to potentially spawn everywhere, and the isBrowsingContextCompatible should be the method "that actually whitelists the browsing contexts that we actually want". Or I am missing something in your suggestion? How would it differ from the current helper beyond restricting it to run only in the content process?

Maybe there will be other checks that we are not aware of yet and would have to run too to ensure that we have a browsing context from a page. So I wonder if there is a way to just ignore every unrelated browsing context by doing just a single check. Maybe there is also a principal (or whatever else) available that could be checked instead? Means I'm more in favor of a whitelist and not a blocklist for content scope.

Oh you meant about the implementation. As long as this is tested, I don't mind. Back at https://bugzilla.mozilla.org/show_bug.cgi?id=1742491#c7 the suggestion to filter contexts for web context was to check the messagemanagergroup attribute on the browser element, although that will probably only work from the parent process. Other than this, reminder that we can't rely on isContent. In the long run, having logic to detect webextension/chrome/... would be useful but if there's a simpler solution at hand, sure.

Comment 5

[Dennis Schubert [:denschub]]

Dennis, are you aware of anything that might have changed?

We do call console.logs() etc inside webpages in cases where we do override/inject something, basically to let developers know that we're doing something here. This usually happens in content scripts. But that's only in cases where we actually do override something, which should never be the case for the web-platform.test domain...

That debug log statement you linked to is inside logic for SmartBlock code, so I'll redirect this needinfo to Tom, who built this. Maybe he can provide some insight!

Comment 6

[Thomas Wisniewski [:twisniewski]]

For SmartBlock we also do some debug-logging while the webcompat add-on starts up (not on release or beta), in addition to also logging on page consoles as Dennis mentioned above.

If there's ultimately a reason for us to avoid logging entirely while running tests, we could complicate the logic for the console output here:

https://searchfox.org/mozilla-central/source/browser/extensions/webcompat/lib/shims.js#21

Of course if we cannot make that decision in JS (in the addon's background page) then we're stuck on that idea.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Thomas! So basically we will have to exclude handling of any console API usage by default in WebDriver BiDi unless it's set to chrome scope (not done yet). So I don't think that there is any need for you to change the code. I only wanted to bring this up given that we haven't seen such logging before and the tests run quite a while for each build on autoland and mozilla-central. So maybe there is some kind of regression or race condition that we? It's really strange that this started out of a sudden last Saturday and specifically for Linux Ccov builds only:

https://treeherder.mozilla.org/jobs?repo=mozilla-central&searchStr=wd1&fromchange=61491ef8a39c90992e711ff57d95f639cd00d506

Julian, would you like to get your proposal added? I think that we can check later if we can find a whitelist solution.

Comment 8

[Julian Descottes [:jdescottes]]

Sure, let me add a test and send to review.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1752292

Comment 10

[Julian Descottes [:jdescottes]]

I'm not sure we actually get WebExtension browsing contexts with our broadcast code. As we have seen for the Android issue, part of the filtering also comes from _getBrowsingContexts in FrameTransport. Given that those logs come from a background page, I don't think our current broadcast code could lead to that. So maybe this is rather linked to our JSWindowActor definition. I will also try to restrict them to messageManagerGroups: ["browsers"]

Comment 11

[Julian Descottes [:jdescottes]]

The issue seems to come from the JSWindowActors definition:

• with messageManagerGroups: ["browsers"] https://treeherder.mozilla.org/jobs?repo=try&revision=3f39252dc69b81ce2234e9a8367db9ef27dce43a
• baseline https://treeherder.mozilla.org/jobs?repo=try&revision=ea767859b0c4d7b845cecbd2a4009375aeb78052

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Interesting. So how would that work when chrome scope is requested? I assume this would have to be lifted and the actors need to be re-registered? I wonder which side-effects that actually has as we have seen for Marionette actors when we tried to unregister them while Firefox still continued to run afterward.

Is there a way to filter out other groups at a later time? Do we have such information on browsing contexts or even the JSWindowActor implementation, which could then be used to not initialize the WindowGlobalMessageManager?

Comment 13

[Julian Descottes [:jdescottes]]

When chrome scope is requested, we allow to create the JSWindowActors everywhere, and filter them dynamically with the same method used for the broadcast.

This is why FrameContextUtils::isBrowsingContextCompatible needs to run both in parent and content.

My initial patches for MessageHandler used such an approach to have a consistent logic between actor registration and broadcast, but we decided to go for a simpler solution for the beginning.

Comment 14

[Julian Descottes [:jdescottes]]

See an early version of the MessageHandler patches: https://phabricator.services.mozilla.com/D120585?id=465567

Comment 15

[Julian Descottes [:jdescottes]]

Attachment: Bug 1753939 - [remote] Do not create message handlers for webextension contexts

Comment 16

[Julian Descottes [:jdescottes]]

Attachment: Bug 1753939 - [remote] Filter out webextensions in FrameContextUtils::isBrowsingContextCompatible

Depends on D138523

Given our current broadcast logic, we should not stumble on webextension contexts during broadcast.
This is because getAllBrowsingContextsInSubtree will not cross process boundaries.

Nevertheless, adding explicit support for WebExtensions in our filter method should be ok, with the added test.
Whenever we lift the JSWindowActor restrictions and rely on isBrowsingContextCompatible for filtering this will help avoid regressions.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/088e8f80fdd9
[remote] Do not create message handlers for webextension contexts r=webdriver-reviewers,rpl,whimboo
https://hg.mozilla.org/integration/autoland/rev/23b8104835f1
[remote] Filter out webextensions in FrameContextUtils::isBrowsingContextCompatible r=rpl,webdriver-reviewers,whimboo

Comment 18

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/088e8f80fdd9
https://hg.mozilla.org/mozilla-central/rev/23b8104835f1

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Julian, would this patch be needed on beta? I assume that this behavior only exists on non-release channels or?

Comment 20

[Julian Descottes [:jdescottes]]

Since BiDi and corresponding tests are only enabled in Nightly , we can wontfix for 98.