Closed Bug 1551278 Opened 8 months ago Closed 6 months ago

Content messaging delegate receives messages from all extensions

Categories

(GeckoView :: General, defect, P1)

Unspecified
Android
defect

Tracking

(firefox68 wontfix, firefox69 fixed, firefox70 fixed)

RESOLVED FIXED
mozilla70
Tracking Status
firefox68 --- wontfix
firefox69 --- fixed
firefox70 --- fixed

People

(Reporter: Agi, Assigned: Agi)

Details

Attachments

(1 file)

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)

Agi says he has a fix.

OS: All → Android
Priority: -- → P1
Attachment #9074044 - Attachment description: Bug 1551278 - WIP: Make WebExtension listeners per-extension. → Bug 1551278 - Make WebExtension listeners per-extension.
Pushed by asferro@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/715bf6e850b8
Make WebExtension listeners per-extension. r=snorp
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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.

Flags: needinfo?(agi)

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

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

Flags: needinfo?(csadilek)

(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 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:
Attachment #9074044 - Flags: approval-mozilla-beta?

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

GeckoView-only WebExtension patch. Approved for Beta69.

Attachment #9074044 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Looks like this needs a rebased patch for Beta uplift.

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