1652487 - BrowserUsageTelemetry.jsm: SyntaxError: Element.closest: '#widget:[addonid]@jetpack-[widgetid]' is not a valid selector

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Rob Wu [:robwu]]

I opened the global JS console and see the following error:

SyntaxError: Element.closest: '#widget:[addonid]@jetpack-[widgetid]' is not a valid selector BrowserUsageTelemetry.jsm:900
    _getWidgetID resource:///modules/BrowserUsageTelemetry.jsm:900
    _recordCommand resource:///modules/BrowserUsageTelemetry.jsm:1032
    _addUsageListeners resource:///modules/BrowserUsageTelemetry.jsm:1047

The code here in browser/modules/BrowserUsageTelemetry.jsm assumes that the input ID is a valid CSS selector. IDs do not have to be a valid CSS selector, to fix this CSS.escape should be used first.

This ID is somehow present in my xulstore.json, and originates from a Jetpack add-on that used the sdk/widget module - https://github.com/mozilla/addon-sdk/blob/e5ce4a3c956327c25e48d49754e09bac95b0897d/lib/sdk/widget.js#L701-L702

Comment 1

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

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

Comment 2

[Dave Townsend [:mossop]]

(In reply to Rob Wu [:robwu] from comment #0)

CSS.escape

TIL!

Comment 3

[Dave Townsend [:mossop]]

Attachment: Bug 1652487: Escape widget identifiers before using them as CSS selectors. r=Gijs!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51d35deafcbb
Escape widget identifiers before using them as CSS selectors. r=Gijs

Comment 5

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/51d35deafcbb

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact which justifies backport consideration here?

Comment 7

[Dave Townsend [:mossop]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)

Is there a user impact which justifies backport consideration here?

So I don't think there is user impact. I do think it would be worth uplifting this to beta though. It breaks this interaction telemetry almost entirely for users in this state. Admittedly that number of users is probably small, but the fix is also trivial and safe.

Comment 8

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

The patch landed in nightly and beta is affected.
:mossop, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 9163900 [details]
Bug 1652487: Escape widget identifiers before using them as CSS selectors. r=Gijs!

Beta/Release Uplift Approval Request

• User impact if declined: None however we lose some telemetry for those affected, that is likely a small number of folks though.
• 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): The patch just adds some additional escaping to strings, there is no risk of regression.
• String changes made/needed:

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9163900 [details]
Bug 1652487: Escape widget identifiers before using them as CSS selectors. r=Gijs!

Seems like a simple-enough Telemetry fix. Approved for 79.0rc1 and 78.1esr.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7933a5c5bdc0

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/0b2ec5d21b06