1551278 - Content messaging delegate receives messages from all extensions

Status: RESOLVED FIXED

(GeckoView :: Extensions, defect, P1)

Description

[[ex-Mozilla] Agi Sferro | :agi]

I noticed this while working on x86_64 test support.

Our Content messaging delegate doesn't check for which WebExtension is supposed to receive messages and just dispatches all of them, this is incorrect as each WebExtension should have it's own delegate.

We should change GeckoSession.setMessageDelegate from:

@AnyThread
public void setMessageDelegate(@Nullable WebExtension.MessageDelegate delegate,
                               @NonNull String nativeApp)

to:

@AnyThread
public void setMessageDelegate(@Nullable WebExtension.MessageDelegate delegate,
                               @NonNull WebExtension webExtension,
                               @NonNull String nativeApp)

Comment 1

[Chris Peterson [:cpeterson]]

Agi says he has a fix.

Comment 2

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1551278 - Make WebExtension listeners per-extension.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/715bf6e850b8
Make WebExtension listeners per-extension. r=snorp

Comment 4

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/715bf6e850b8

Comment 5

[Chris Peterson [:cpeterson]]

Agi, do we need to uplift this fix to GV 69 Beta? Does Fenix/A-C need this fix? They will be updating to GV 69 Beta in a couple weeks.

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

It probably wouldn't hurt to uplift. Christian does Fenix/Reader mode need this?

Comment 7

[Christian Sadilek [:csadilek]]

Yes, I'd vote for uplifting to 69 beta, please.

Currently Fenix only has one web extension and can't run into this problem. However, there's an FxA extension in the works which will make use of this API and we shouldn't send its messages to other extensions.

Comment 8

[Chris Peterson [:cpeterson]]

(In reply to Christian Sadilek [:csadilek] from comment #7)

Yes, I'd vote for uplifting to 69 beta, please.

In that case, beta 69=affected.

Comment 9

[[ex-Mozilla] Agi Sferro | :agi]

Comment on attachment 9074044 [details]
Bug 1551278 - Make WebExtension listeners per-extension.

Beta/Release Uplift Approval Request

• User impact if declined: All built in WebExtensions in Fenix will broadcast messages to each other, potentially executing code in unintended ways.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch only affects GeckoView consumers using WebExtensions, no other code path is affected. The worst case scenario is that features that rely on WebExtensions are broken (currently ReaderMode and favicons in Fenix).
• String changes made/needed:

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9074044 [details]
Bug 1551278 - Make WebExtension listeners per-extension.

GeckoView-only WebExtension patch. Approved for Beta69.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Looks like this needs a rebased patch for Beta uplift.

Comment 12

[[ex-Mozilla] Agi Sferro | :agi]

https://hg.mozilla.org/releases/mozilla-beta/rev/da83a38eed7117793dbd1c64e53d4aab10635095

Comment 13

[Chris Peterson [:cpeterson]]

Moving some extension bugs to the GeckoView::Extensions component.