1887029 - Restrict tab attributes TabAttributes can restore

Status: RESOLVED FIXED

(Firefox :: Session Restore, task)

Description

[Nika Layzell [:nika] (ni? for response)]

The TabAttributes code in SessionStore is currently designed to be able to save & restore arbitrary attributes on tabs in the browser, however it is only ever used to restore a single attribute ("customizemode") outside of tests. We should simplify this code and only allow it to save & restore supported attributes.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1887029 - Simplify TabAttributes to explicitly specify supported attributes, r=#sessionstore-reviewers!

The only tab attribute which is ever persisted by SessionStore is
"customizemode". This patch limits the logic to only allow persisting and
restoring this attribute.

The browser_attributes.js test is also updated to use the "customizemode"
attribute for testing, rather than a custom specified attribute.

Depends on D205480

Comment 2

[Sam Foster [:sfoster] (he/him)]

Adding the other peers to cc so we can discuss this patch.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07f554ea4869
Simplify TabAttributes to explicitly specify supported attributes, r=sessionstore-reviewers,sfoster,dao

Comment 4

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8daf286173c4
Backed out 3 changesets (bug 1887029, bug 1886892) for causing failures at browser_all_files_referenced.js. CLOSED TREE

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out (bug 1887029, bug 1886892) for causing failures at browser_all_files_referenced.js:
https://hg.mozilla.org/integration/autoland/rev/8daf286173c44985a773a83871e29d29f8e08cd8

Push with failures
Failure log

browser/base/content/test/static/browser_all_files_referenced.js | unreferenced file: resource://gre/modules/sessionstore/Utils.sys.mjs

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8ff1a8679fb
Simplify TabAttributes to explicitly specify supported attributes, r=sessionstore-reviewers,sfoster,dao

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e8ff1a8679fb

Comment 8

[Donal Meehan [:dmeehan]]

:nika this grafts cleanly to esr115.
Could you please add an esr115 uplift request?

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9392813 [details]
Bug 1887029 - Simplify TabAttributes to explicitly specify supported attributes, r=#sessionstore-reviewers!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Browser security improvements
• User impact if declined: Higher potential for TabAttributes code abuse in ESR
• Fix Landed on Version: 126
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Changes to sessionstore code are always somewhat risky, but this is a well scoped change to reduce the functionality of a previously-exploited component to only the features actually used by the browser.

Comment 10

[Donal Meehan [:dmeehan]]

Comment on attachment 9392813 [details]
Bug 1887029 - Simplify TabAttributes to explicitly specify supported attributes, r=#sessionstore-reviewers!

Approved for 115.11esr.

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/39f61fc5d16c

Comment 12

[Donal Meehan [:dmeehan]]

Backed out from esr115 for causing test failures
https://hg.mozilla.org/releases/mozilla-esr115/rev/bcebc3285ade3fd44e9ca2dd5e1601486799b234

Push: https://treeherder.mozilla.org/jobs?repo=mozilla-esr115&duplicate_jobs=visible&group_state=expanded&searchStr=browser-chrome&revision=f5c52bd3fbadd8f5ba8d88e80f36cbb629759417
Example: https://treeherder.mozilla.org/logviewer?job_id=455846407&repo=mozilla-esr115&lineNumber=3169

Comment 13

[Andrew McCreight [:mccr8]]

It looks like createAboutBlankDocumentViewer (from browser_attributes.js) is called createAboutBlankContentViewer on esr115. I'm not sure if there are other issues.

Comment 14

[Andrew McCreight [:mccr8]]

I'll check locally if that's sufficient to fix the test, as Nika is out.

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: Bug 1887029 - Simplify TabAttributes to explicitly specify supported attributes, , a=dmeehan

The only tab attribute which is ever persisted by SessionStore is
"customizemode". This patch limits the logic to only allow persisting and
restoring this attribute.

The browser_attributes.js test is also updated to use the "customizemode"
attribute for testing, rather than a custom specified attribute.

Minor test fix for ESR115.

Comment 16

[Andrew McCreight [:mccr8]]

I confirmed with a try push that browser_attributes.js passes with the new version of the patch. I just replaced one instance of createAboutBlankDocumentViewer with createAboutBlankContentViewer in the test.

Comment 17

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

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 18

[Andrew McCreight [:mccr8]]

This is ready to reland on esr115 with the new version of the patch I attached ( https://phabricator.services.mozilla.com/D208655 ). Thanks.

Comment 19

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/76c491d2246b