1173857 - Tabs are sometimes restored with invalid proxy state

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

STR:

0) Have automatic session restore enabled.
1) Have a window with two tabs.
2) Select the first tab.
3) Quit/Start Firefox two times.
4) Select the second tab.

The globe/TLS icon should be clickable but is instead greyed out.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1173857-Don-t-collect-.userTypedValue-for-tabs-t.patch

We must not collect .userTypedValue from pending tabs. If it's already in the tabData that's fine.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Hmm, I thought we had code to cover this. Why isn't this working?
https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#2896

It looks like we eventually clear tabData.userTypedValue, but only after we fill in browser.userTypedValue. And by that time there's no reason to avoid collecting browser.userTypedValue.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Bill McCloskey (:billm) from comment #2)
> Hmm, I thought we had code to cover this.

Yes, you're right. I forgot about this it seems, sorry for that.

> It looks like we eventually clear tabData.userTypedValue, but only after we
> fill in browser.userTypedValue. And by that time there's no reason to avoid
> collecting browser.userTypedValue.

We do fill in userTypedValue but we always do that, even if the user didn't modify the URL in the location bar - this is some wallpaper fix from ages ago. On "restoreTabContentStarted" we set the userTypedValue again if needed and only now we can clear the cached value and start to collect it.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1173857-Don-t-collect-.userTypedValue-for-tabs-t.patch, v2

TabStateCache.set() already handles |null| values and deletes entries accordingly. TabState.copyFrameCache() shouldn't check for |if (value)| because the cache already did that and we'll treat |0| and |""| as non-values too.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1163745-TabAttributes.set-should-ignore-internal.patch

The only test that broke was browser_attributes.js. Turns out that although TabAttributes.jsm should ignore internal attributes it checks for exactly the wrong behavior (my fault I guess). Fixed the test and TabAttributes.set() to correctly ignore internal attributes.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b148ba60f686

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5ab32a115a62

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b148ba60f686
https://hg.mozilla.org/mozilla-central/rev/5ab32a115a62