Closed Bug 996053 Opened 10 years ago Closed 10 years ago

make nsISessionStore's setTabValue, setWindowValue and setGlobalValue take arbitrary JS values but throw for non-strings, for consistency with SessionStore.jsm

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 33
Tracking Status
firefox29 --- unaffected
firefox30 - wontfix
firefox31 - wontfix

People

(Reporter: smacleod, Assigned: dao)

References

Details

(Keywords: addon-compat, dev-doc-needed, Whiteboard: p=1 s=33.1 [qa-])

Attachments

(1 file)

After Bug 908440, nsISessionStore and SessionStore.jsm respond differently to non-string values passed to setWindowState, setTabState, and setGlobalState.

nsISessionStore silently converts objects into an empty string (and did before as well), where SessionStore.jsm will throw an exception.

At least one add-on is affected by this (KeeFox). Our docs also state "You may use any JavaScript object as the data" [1]. This documentation is referring to nsISessionStore, so it is technically correct, but not practical because any JavaScript object you use is converted to an empty string if it is not a string.

As a more backwards compat fix we could discard non string objects passed to SessionStore.jsm, converting them to empty strings as well. Really though this behavior is misleading, and not documented.

Tim, do you have any thoughts on this?


[1] https://developer.mozilla.org/en/docs/Session_store_API#Saving_a_value_with_a_tab .
Flags: needinfo?(ttaubert)
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
I think we should indeed aim for less disruption here and just accept all values as before, converting them to an empty string if needed. Just like for deprecated methods it would be good to report to the console so that add-on authors can fix their code.
Flags: needinfo?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #1)
> I think we should indeed aim for less disruption here and just accept all
> values as before, converting them to an empty string if needed. Just like
> for deprecated methods it would be good to report to the console so that
> add-on authors can fix their code.

Hi Tim, 

What impact would this have on our users if decided not to track or block?
Flags: needinfo?(ttaubert)
(In reply to Benjamin Kerensa [:bkerensa] from comment #2)
> What impact would this have on our users if decided not to track or block?

Hey Benjamin, the impact might be that we break a few add-ons out there. I haven't done any research on how many add-ons would be affected but at least one is (KeeFox) as reported in bug 908440 comment #6.
Flags: needinfo?(ttaubert)
This made it into the backlog, so marking it [diamond]. Tim, are you willing to mentor?
Flags: needinfo?(ttaubert)
Whiteboard: [diamond]
(In reply to Mike Hoye [:mhoye] from comment #4)
> This made it into the backlog, so marking it [diamond]. Tim, are you willing
> to mentor?

I'd be fine to mentor this as well if Tim prefers.
No reason you can't both sign on; whoever gets there first gets there. Adding you as a mentor.
Whiteboard: [diamond] → [diamond] [mentor=smacleod]
(In reply to Steven MacLeod [:smacleod] from comment #5)
> I'd be fine to mentor this as well if Tim prefers.

SGTM!
Flags: needinfo?(ttaubert)
As a mentored bug, and without a lot of signs of major breakage, we don't need to track this but can reconsider if there does turn out to be significant impact to addons.
Whiteboard: [diamond] [mentor=smacleod] → [diamond] [mentor=smacleod] p=0 [qa?]
Whiteboard: [diamond] [mentor=smacleod] p=0 [qa?] → [diamond] [mentor=smacleod] p=0 [qa-]
(In reply to Tim Taubert [:ttaubert] from comment #1)
> I think we should indeed aim for less disruption here and just accept all
> values as before, converting them to an empty string if needed. Just like
> for deprecated methods it would be good to report to the console so that
> add-on authors can fix their code.

Having read both bugs, I'm not clear on what needs to happen here. Does this essentially want the code inside the typeof checks added in bug 908440 to instead do a string conversion on the object if it's not already a string?
Flags: needinfo?(ttaubert)
(note also that we're too late for 30 now - does that mean maybe it makes more sense to leave this as-is and just update the docs?)
(In reply to :Gijs Kruitbosch from comment #9)
> Having read both bugs, I'm not clear on what needs to happen here. Does this
> essentially want the code inside the typeof checks added in bug 908440 to
> instead do a string conversion on the object if it's not already a string?

Yes.

(In reply to :Gijs Kruitbosch from comment #10)
> (note also that we're too late for 30 now - does that mean maybe it makes
> more sense to leave this as-is and just update the docs?)

Maybe, I still think we could fix it. Not failing in future versions doesn't make it any worse but it would be good to have some consistency between the two ways to access the API.
Flags: needinfo?(ttaubert)
Hardware: x86 → All
Whiteboard: [diamond] [mentor=smacleod] p=0 [qa-] → [good-first-bug][mentor=smacleod][lang=js] p=1 [qa-]
Assignee: nobody → dao
Status: NEW → ASSIGNED
Whiteboard: [good-first-bug][mentor=smacleod][lang=js] p=1 [qa-] → [good-first-bug][mentor=smacleod][lang=js] p=1 s=33.1 [qa-]
(In reply to Tim Taubert [:ttaubert] from comment #1)
> I think we should indeed aim for less disruption here and just accept all
> values as before, converting them to an empty string if needed.

What does "if needed" mean here? How would you treat numbers (this includes stuff like NaN)? Should the idl magic completely be replicated here? I'm not sure that's desirable and worthwhile.
Flags: needinfo?(ttaubert)
Whiteboard: [good-first-bug][mentor=smacleod][lang=js] p=1 s=33.1 [qa-] → p=1 s=33.1 [qa-]
(In reply to Dão Gottwald [:dao] from comment #12)
> What does "if needed" mean here? How would you treat numbers (this includes
> stuff like NaN)? Should the idl magic completely be replicated here? I'm not
> sure that's desirable and worthwhile.

I agree that mirroring idl magic here doesn't seem attractive but it's the only way to ensure the same behavior between using nsISessionStore and loading SessionStore.jsm, without breaking any clients.

Another option might be to convert |in AString| params for those methods to |in jsval|. When passing objects we would then throw errors as we do now when using the JSM directly. This might break backwards compat with a few add-ons but passing anything other than a string was never supported anyway.

Now that we shipped 30 and would break misbehaving add-ons already the last suggestion sounds like the better way to go.
Flags: needinfo?(ttaubert)
Summary: [Session Restore] setWindowValue and setTabValue no longer accept arbitrary JS values → make nsISessionStore's setTabValue, setWindowValue and setGlobalValue take arbitrary JS values but throw for non-strings, for consistency with SessionStore.jsm
Attached patch patchSplinter Review
Attachment #8441939 - Flags: review?(ttaubert)
No longer blocks: 961646
Comment on attachment 8441939 [details] [diff] [review]
patch

Review of attachment 8441939 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

We'll need to fix browser_524745.js because that fails now. This is as simple as doing:

ss.setWindowValue(window_B, uniqKey, uniqVal.toString());
Attachment #8441939 - Flags: review?(ttaubert) → review+
https://hg.mozilla.org/mozilla-central/rev/f21839faba8b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
I know, no surprise but this breaks the addon tabgroupsmanager.

TabGroupsManager.session.sessionStore.setTabValue(tab,"TabGroupsManagerGroupId",this.id);

Here this.id type would be type number.
I apologyze for my ignorance but, how am i supossed to apply this patch on my Windows pc?

Thanks.

Manusoftar®.-
Hi, Manusoftar - 

This has been resolved, so it looks like it's already included in Firefox 33. If you're interested in being able to build Firefox and test patches generally, you'll have to set up a build environment.

Here is some good documentation that can get you started, and it includes some information about where to turn if you have questions.

https://developer.mozilla.org/en-US/docs/Introduction 

Thanks for your interest!
The simplest way would be to update to Firefox 33. Firefox 33 is the current stable release. it is available for download at https://www.mozilla.org/en-US/firefox/all/
First of all thanks for the fast answer. 

Second, if version 33 is supossed to have this patch included then i have to advise that it didn't work for me, i'm currently running version 33.0.2 and i still have this error


setTabValue only accepts string values
	
SessionStore.jsm (line 1748)<System>
[Exception... "[JavaScript Error: "setTabValue only accepts string values" {file:                  "resource:///modules/sessionstore/SessionStore.jsm" line: 1748}]'[JavaScript Error: "setTabValue only accepts string values" {file: "resource:///modules/sessionstore/SessionStore.jsm" line: 1748}]' when calling method: [nsISessionStore::setTabValue]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: chrome://alx_ns_ph/content/overlay.js :: ALX_NS_PH_SPARKLINE.logStautsCode :: line 58" data: yes]

I would appreciate if you could reopen this report.

Thanks. MAnusoftar®.-
We're deliberately throwing an exception for values that aren't strings. Please file a new bug if you think we should change that.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: