908440 - [Session Restore] Add string type checks for values passed to Global, Tab, and Window value storage API

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Steven MacLeod [:smacleod]]

The get*Value(...) and set*Value(...) (For Global, Tab, and Window) methods on SessionStore should check their parameters to ensure only strings are passed as values. These methods aren't protected by the idl type checks and consumers should not be able to pass in objects and maintain references to them.

Comment 2

[Steven MacLeod [:smacleod]]

Attachment: Patch - Add type checks to the set* methods

Comment 3

[Steven MacLeod [:smacleod]]

Attachment: Patch - Add type checks to the set* methods v2

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[Chris Tomlinson]

The MDN docs clearly state that "You may use any JavaScript object as the data." (https://developer.mozilla.org/en/docs/Session_store_API)

Hence, this patch causes unhandled exceptions in my add-on (KeeFox). Perhaps others are affected too (I doubt it's practical to automatically detect the scale of the issue).

Given the current focus on FF29/Australis by add-on authors, it would be good if this patch could be:

1) modified to output a warning instead of throwing an exception
2) delayed for a version or two
3) more widely announced to add-on authors, along with appropriate documentation modifications

In my case, I am actually setting a string most of the time but occasionally set "undefined". It's trivial for me to change this to an empty string but maybe the change for others could be more difficult.

Alternatively, if the API has never correctly stored and returned non-strings, could the set functions just check for undefined? (and null?)

I don't know if undefined and null count as references from the perspective of this API module. If they don't, presumably they could be permitted to be set as per current behaviour without any impact on GC?