Closed Bug 1753939 Opened 3 years ago Closed 2 years ago

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

Categories

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

Firefox 98
defect
Points:
2

Tracking

(firefox-esr91 unaffected, firefox97 unaffected, firefox98 wontfix, firefox99 fixed)

RESOLVED FIXED
99 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox97 --- unaffected
firefox98 --- wontfix
firefox99 --- fixed

People

(Reporter: whimboo, Assigned: jdescottes)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, Whiteboard: [bidi-m3-mvp])

Attachments

(2 files)

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.

Flags: needinfo?(dschubert)
Keywords: regression

(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.

Blocks: 1753853

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?

Blocks: 1753855
Blocks: 1753906
Blocks: 1753854
Blocks: 1753856
Blocks: 1753857

(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.

Has Regression Range: --- → yes

(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.

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!

Flags: needinfo?(dschubert) → needinfo?(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.

Flags: needinfo?(twisniewski)

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.

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

Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

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

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"]

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?

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.

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

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.

Points: --- → 2
Priority: -- → P2
Whiteboard: [bidi-m3-mvp]
Pushed by jdescottes@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 99 Branch

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

Flags: needinfo?(jdescottes)

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

Flags: needinfo?(jdescottes)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: