1838448 - CPU usage in StructuredCloneHolder.deserialize starting version 115 far higher than in 114 with Dashlane extension

Status: NEW

(WebExtensions :: Storage, defect)

Description

[Tristan P]

Steps to reproduce:

• Interact on a webpage with an extension that communicates to a background page

Hello,
I work at Dashlane and we noticed a latency in our product since 115.

On 115 we notice a latency: https://share.firefox.dev/442n3Pj
On 114 all seems good https://share.firefox.dev/3N5xAlN

Initially posted in https://bugzilla.mozilla.org/show_bug.cgi?id=1833016#c17

Actual results:

On version 115, we notice an increase of CPU in StructuredCloneHolder.deserialize. This can slow down our web extension.
We suspect it happens during runtime.postMessage / or port.postMessage

Performance profiling here https://share.firefox.dev/442n3Pj

I do not have a simple open-source reproduction repository yet.

Expected results:

On version 114 all seems good https://share.firefox.dev/3N5xAlN

Comment 1

[Andrew McCreight [:mccr8]]

Kris, the reporter suspects bug 1769763 could have caused this. It does seem plausible given the time in the profile is being spent in StructuredCloneHolder deserialize. Could you take a look? Thanks.

Comment 2

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

The bug has a release status flag that shows some version of Firefox is affected, thus it will be considered confirmed.

Comment 3

[Kris Maglione [:kmag]]

(In reply to Andrew McCreight [:mccr8] from comment #1)

Kris, the reporter suspects bug 1769763 could have caused this. It does seem plausible given the time in the profile is being spent in StructuredCloneHolder deserialize. Could you take a look? Thanks.

I don't think that's possible. That change shouldn't affect StructuredCloneHolder.deserialize at all. It should really only affect creation and sending between processes. And the bulk of the overhead in js::AtomizeString, which wouldn't apply to the debug names at all.

Comment 4

[Donal Meehan [:dmeehan]]

:fdoty could this be triaged for Priority/Severity?

Comment 5

[Andrew McCreight [:mccr8]]

It can be a little tedious, but if you can't come up with a test case we can try, you could use MozRegression to figure out what caused the regression. Finding a regressor is usually a good step towards getting some forward movement on a mysterious issue. Thanks.

Comment 6

[Pascal Chevrel:pascalc]

Tristan, did you have the opportunity to try and find the regression range with mozregression? Thanks

Comment 7

[Tristan P]

No I did not have the time yet. The workflow takes 3 minutes to execute, I attempted to create an easiest repro without success so far ( it looks like doing postMessage "ping" between the background page and the content script isn't enough to see the issue)

Comment 9

[Kestrel]

STR:

1. Visit https://webmail.gandi.net/roundcube/ with Dashlane installed.
2. Enable performance capture.
3. Click Dashlane icon in login field.
4. Click "Add a new login" and let tab open.
5. Stop performance capture.

Dashlane tab opens and does work with significant StructuredCloneHolder.deserialize in webextension process that did not appear previously.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=21476ba58834885fe3b0ca8b9a916f0ed57616de&tochange=532904ff2fffa0332a2d6136792b03a9ae6bcac1

Regressed by Bug 1823713.

Comment 10

[Pascal Chevrel:pascalc]

Thanks Kestrel for the regression range!

Tomislav, it seems that bug 1823713 caused this regression, would backing it out in a 115 dot release be a feasible option or do we need a follow up fix on the release branch? Thanks

Comment 11

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

The product::component has been changed since the backlog priority was decided, so we're resetting it.
For more information, please visit BugBot documentation.

Comment 12

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

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

Comment 13

[Tristan P]

Thank you Kestrel for the mozregression usage. I tried to do it at work, but our proxy/cloudflare setting is making this complicated.
So I think you helped me a lot to understand what is going on.

TL-DR: Dashlane is using storage.session when available, to avoid losing data due to service worker termination. Even in MV2.

We already have a fix: we wont use storage.session in Mv2.

Now, I am curious why persisting data in the session storage seems significant, CPU-wise, and what can we do to help.
Writing base-64 in it, maybe?

Comment 14

[Rob Wu [:robwu]]

Tristan - are you using storage.onChanged? If so, you may be affected by bug 1842009.

To avoid the issue, register onChanged on the specific storage areas of interest, e.g.

NOT:

• storage.onChanged

Use one or all of these as appropriate:

• storage.local.onChanged
• storage.sync.onChanged
• storage.session.onChanged (only if interested in the data; doesn't work in content scripts).

Comment 15

[Tristan P]

Ha! We finally pushed a new version of our extension, making it stop using the session storage in mv2. So I cannot reproduce the issue in mv2 anymore.

We only do storage.area.onChanged. We shouldnt be affected by #1842009.
I see however that we do multiple calls to it in mv3, I am going to investigate the impact of that and make sure we have only a single listener if this matters.

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

The Performance Impact Calculator has determined this bug's performance impact to be low. If you'd like to request re-triage, you can reset the Performance Impact flag to "?" or needinfo the triage sheriff.

Platforms: macOS
Impact on browser: Causes noticeable jank
Configuration: Specific but common
Resource impact: Some