Session store should handle exceptions when manipulating sessionStorage

RESOLVED FIXED in Firefox 3.1b2

Status

()

Firefox
Session Restore
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

Trunk
Firefox 3.1b2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
The code landed in bug 339445 as part of attachment 340154 [details] [diff] [review] does not perform enough exception handling.  It can throw both when storage is turned off, or inside the private browsing mode.

The discussion in bug 458498 is relevant here as well.  The final decision there was to continue throwing exceptions from DOMStorage, and handling them properly inside the session store code.
(Assignee)

Comment 1

9 years ago
Created attachment 342152 [details] [diff] [review]
Patch (v1)

Simply add try/catch blocks around the sessionStorage calls.
Attachment #342152 - Flags: review?(dietrich)

Comment 2

9 years ago
Comment on attachment 342152 [details] [diff] [review]
Patch (v1)

These try-catch blocks aren't specific enough:

AFAICT in sss_serializeSessionStorage only the line containing |storage.length| can actually through (as all the other Storage related calls are already wrapped in a try-catch-block) and in sss_deserializeSessionStorage all the Storage related calls are already wrapped in a try-catch-block as well - except for getSessionStorageForURI which doesn't throw, as we've already established.

Am I missing something? If so, please point out which other calls might also throw.
Attachment #342152 - Flags: review-

Comment 3

9 years ago
(In reply to comment #2)
> getSessionStorageForURI which doesn't throw, as we've already established.

Looks like it could throw as well. At least, it doesn't seem to like jar: URLs (maybe generally URIs with more than one scheme). So please add that line to the try-catch-block during serialization and guard specifically against this exception during deserialization.

Comment 4

9 years ago
(In reply to comment #3)
> guard specifically against this exception during deserialization.

Then again, there's no way to get invalid URIs at that point, in the first place, or is there?
(Assignee)

Comment 5

9 years ago
(In reply to comment #4)
> Then again, there's no way to get invalid URIs at that point, in the first
> place, or is there?

There are other possibile cases where getSessionStorageForURI would throw (for example, if it can't create the session storage component for whatever reason).  I think it would be a good idea to protect against its failures.

Comment 6

9 years ago
(In reply to comment #5)
> if it can't create the session storage component for whatever reason

Why would it not be able to create that component (except for OOM conditions or a broken build)?
(Assignee)

Comment 7

9 years ago
(In reply to comment #6)
> (In reply to comment #5)
> Why would it not be able to create that component (except for OOM conditions or
> a broken build)?

There are many cases in nsDocShell::GetSessionStorageForURI might fail; see: <http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1682>.
(Assignee)

Comment 8

9 years ago
Created attachment 342249 [details] [diff] [review]
Patch (v2)

Simon: does this patch cover your concerns?
Attachment #342152 - Attachment is obsolete: true
Attachment #342249 - Flags: review?(zeniko)
Attachment #342152 - Flags: review?(dietrich)
Hi,

I couldn't apply the current patch (v1)
 
Hunk #1 succeeded at 1127 (offset 33 lines).
Hunk #2 succeeded at 1857 (offset 33 lines).

This is on a fresh hg clone of mozilla-central.

Comment 10

9 years ago
Comment on attachment 342249 [details] [diff] [review]
Patch (v2)

>+      try {
>+        storage = aDocShell.getSessionStorageForURI(uri);
>+      }
>+      catch (ex) {
>+        // sessionStorage might throw if it's turned off, see bug 458954

AFAICT that's wrong. Going through the potential points of failure in getSessionStorageForURI, there are invalid docshells and OOM (would cause failure at an earlier point), and an invalid asciiHost (will go away in bug 455070, though).

Please either fix that comment, or just leave the deserialization as is (my preference). r+=me with that fixed.
(Assignee)

Comment 11

9 years ago
(In reply to comment #9)
> I couldn't apply the current patch (v1)

MY bad, I forgot to pass -u to hg pull, I guess.  I'm updating my repo and will post a new patch.

BTW, if the only output from the patch utility were those that you quoted, the patch has been applied successfully using a fuzzy match.
(Assignee)

Comment 12

9 years ago
Created attachment 342253 [details] [diff] [review]
Patch (v3)

(In reply to comment #10)
> Please either fix that comment, or just leave the deserialization as is (my
> preference). r+=me with that fixed.

Sure, let's just handle serialization here.

Aaron: this should apply cleanly.
Attachment #342249 - Attachment is obsolete: true
Attachment #342253 - Flags: review?(zeniko)
Attachment #342249 - Flags: review?(zeniko)

Comment 13

9 years ago
Comment on attachment 342253 [details] [diff] [review]
Patch (v3)

>+        storageItemCount = storage.length;
>+        if (!storage || storageItemCount == 0)

One thing I've missed: There's no point in testing whether |storage| is |null| if you've already used it. Since we catch all exceptions, anyway, you can just drop the |!storage| bit.

Thanks and r+=me.
Attachment #342253 - Flags: review?(zeniko) → review+
(Assignee)

Comment 14

9 years ago
(In reply to comment #13)
> One thing I've missed: There's no point in testing whether |storage| is |null|
> if you've already used it. Since we catch all exceptions, anyway, you can just
> drop the |!storage| bit.

Oops, you're right.

I'll fix this when landing.  I moved this to the bottom of my mq, and now I just need to wait for the tree to open to get this landed.  Thanks for your review.
(Assignee)

Comment 15

9 years ago
Pushed to mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/1d719b917629>
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b2
You need to log in before you can comment on or make changes to this bug.