User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2 (.NET CLR 3.5.30729) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090221 Minefield/3.2a1pre (.NET CLR 3.5.30729) As extensions can modify the value returned by the _readStateFile function in the SessionStart compoment, it would be a good idea to wrap the returned result in parenthesis before trying to call the evalInSandbox function. Otherwise if an extension returns a true JSON string (one created by JSON.stringify and not wrapped in parenthesis) the evalinSandbox call with throw an exception. Since the SessionStore component also reads the _iniString variable from SessionStart, either the _iniString variable itself should contain the parenthesis wrapped string or the evalinSandbox call in SessionStart and the _safeeval() call in SessionStore's init function should explicitly wrap it. The main reason I ask for this is to keep things consistent: 1. Firefox's SessionStore component will wrap API function parameters before attempting to _safeeval() them so I think it SessionStart should as well. 2. Making this change would keep consistency between Firefox and SeaMonkey 2.0a3. Both Firefox and SeaMonkey allow extensions to modify the initial state by observing the "sessionstore-state-read" notification, but as SeaMonkey does a JSON.parse and Firefox does an evalInSandBox, an extension would need to return parenthesis wrapped JSON strings to Firefox and true JSON strings to SeaMonkey to prevent exceptions. This requires adding extra browser detection code to extensions which would otherwise not be needed. Reproducible: Always
Created attachment 363500 [details] [diff] [review] fix Dietrich: We'll want this two-liner on the 1.9.1 branch for future proofing our code (should we ever want to completely switch to JSON) and also for consistency with SeaMonkey (which has already made that switch). The patch is save, as adding parentheses when there's already a pair is a no-op for eval.
Assignee: nobody → zeniko
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #363500 - Flags: review?(dietrich)
Comment on attachment 363500 [details] [diff] [review] fix r=me
Attachment #363500 - Flags: review?(dietrich) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
Am I right in assuming this won't make it into Firefox 3.1 since only the trunk got the fix?
(In reply to comment #4) The patch will land on the Firefox 3.1 branch as soon as we've got approval for doing so by the release drivers (note the approval1.9.1? flag). And in case you're wondering: The Target Milestone refers to the next _Trunk_ milestone to contain the fix, branch milestones are tracked through keywords (fixed1.9.1, verified1.9.1, etc.).
Comment on attachment 363500 [details] [diff] [review] fix a191=beltzner
Attachment #363500 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed → fixed1.9.1
You need to log in before you can comment on or make changes to this bug.