458954 - Session store should handle exceptions when manipulating sessionStorage

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[(no longer active)]

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.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Simply add try/catch blocks around the sessionStorage calls.

Comment 2

[Simon Bünzli]

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.

Comment 3

[Simon Bünzli]

(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

[Simon Bünzli]

(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?

Comment 5

[(no longer active)]

(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

[Simon Bünzli]

(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)?

Comment 7

[(no longer active)]

(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>.

Comment 8

[(no longer active)]

Attachment: Patch (v2)

Simon: does this patch cover your concerns?

Comment 9

[Aaron Train [:aaronmt]]

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

[Simon Bünzli]

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.

Comment 11

[(no longer active)]

(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.

Comment 12

[(no longer active)]

Attachment: 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.

Comment 13

[Simon Bünzli]

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.

Comment 14

[(no longer active)]

(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.

Comment 15

[(no longer active)]

Pushed to mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/1d719b917629>