350718 - [SessionStore] about:config's textbox value isn't restored anymore

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Simon Bünzli]

Not sure where to file this. This bug is made visibly by SessionStore but might rather be a Core issue.

Steps to reproduce:
1. Open about:config and enter text into the filter
2. Close the tab
3. Re-open that tab

Expected result:
The filter text is restored.

This regressed with bug 345991 - before, it was possible to get the value by simply not applying an XPCNativeWrapper to about:config's document.

OTOH if this can't be fixed for Firefox 2.0, the special treatment of about:config should at least marked with a note about why it doesn't work in nsSessionStore.js.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So is the problem that .value is not returning the right string?  Or something else?

Comment 2

[Simon Bünzli]

The problem is that .value is undefined in the first place.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm...  Is that object that we're doing .value on a XUL textbox or something?

Comment 4

[Simon Bünzli]

That's what the DOM Inspector says. And that one is able to get to the value. "Normal" chrome however is not.

Comment 5

[Boris Zbarsky [:bzbarsky]]

DOM inspector doesn't use XPCNativeWrapper.  This means that it's exploitable if you use it on some random site, of course.  ;)

The problem is that .value on XUL textboxes is bound via XBL, so there's no way XPCNativeWrapper can know about it...  Perhaps in the about:config case you simply want to unwrap in this code, if you're Really Sure that content can't touch it (should be true, since about:config is chrome).

Comment 6

[Simon Bünzli]

Attachment: branch patch

Drivers: This low-risk patch fixes the regression by unwrapping about:config's document wherever needed (before bug 345991 was fixed, about:config wasn't wrapped at all - so there should be no new security concerns, see also comment #5 above).

With this patch, about:config's filter is restored after a restart or after reopening a closed tab. This can be a blessing for developers and power users and should if possible be restored.

Comment 7

[Simon Bünzli]

Attachment: trunk patch

Additionally to the branch patch, this patch contains the bits of the patch to bug 349961 which weren't checked in (complete removal of no longer required code concerning XPCNativeWrappers).

Comment 8

[Asaf Romano (gone)]

Comment on attachment 238234 [details] [diff] [review]
branch patch

r=mano. drivers: very low risk.

Comment 9

[Asaf Romano (gone)]

Comment on attachment 238235 [details] [diff] [review]
trunk patch

>Index: browser/components/sessionstore/src/nsSessionStore.js
>===================================================================
{
>+            var data = aBrowser.parentNode.__SS_text[textarea];
>             if (!data.cache) {
>               // update the text element's value before adding it to the data structure
>-              data.cache = encodeURI(data.element.value);
>+              data.cache = encodeURI(textarea.value);
>             }
>             text.push(data.id + "=" + data.cache);
>           }
>         }

Explain this change please (in particular, the use of textarea.value).

Comment 10

[Simon Bünzli]

(In reply to comment #9)
> Explain this change please (in particular, the use of textarea.value).

Currently there are two references to the text areas to be saved: the keys to __SS_text and the .element field of the corresponding object. This patch does away with the redundant .element field and directly uses the reference used as key. So instead of |data.element| we now write |textarea| and thus |data.element.value| becomes |textarea.value|.

Originally data.element held a wrapped instance of the textarea while the unwrapped instance was used as a key. Since tanks to bug 345991 we now automatically get everything wrapped, this dichotomy has become obsolete.

Comment 11

[Simon Bünzli]

branch patch -> RC1 radar

Comment 12

[Mike Connor [:mconnor]]

not a blocker, will evaluate patch separately.

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 238234 [details] [diff] [review]
branch patch

a=mconnor on behalf of drivers for 1.8 branch checkin

Comment 14

[Asaf Romano (gone)]

Comment on attachment 238235 [details] [diff] [review]
trunk patch

Tested in http://forums.mozillazine.org/posting.php?mode=newtopic&f=38

1. Write something in the message-body field.
2. Blur it
3. Crash and reopen the browser

The restored value is "undefined".

Comment 15

[Simon Bünzli]

Attachment: trunk patch

Never mind, please just land the bits which landed on the branch and leave the rest for future (if ever) cleanup. Thanks.

Comment 16

[Asaf Romano (gone)]

Comment on attachment 240656 [details] [diff] [review]
trunk patch

r=mano

Comment 17

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/sessionstore/src/nsSessionStore.js 	1.52

Is this now FIXED?

Comment 18

[Simon Bünzli]

Adjusting the summary to make this bug FIXED.

Note to self: Depending on the fix to bug 355766 we might want to adjust the solution chosen here.

Comment 19

[Michael Kohler [:mkohler]]

Verified with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090618 Minefield/3.6a1pre ID:20090618031329

Comment 20

[u279076]

in-litmus+
https://litmus.mozilla.org/show_test.cgi?id=7791