524178 - sessionrestore.js is read in 1kb chunks

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[(dormant account)]

Given that this file can get quite large, it would be best if the code would call ->available and then read the whole length of the file. Individual read() syscalls can be quite slow on wince/osx

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

For easy linkage: http://hg.mozilla.org/mozilla-central/file/be5eeaf56443/browser/components/sessionstore/src/nsSessionStartup.js#l280

So since I'm not familiar with all the file APIs, what's actually happening?

I see cvstream.init with a 1kb buffer, and then cvstream.readString reading 4096 characters. Does init read the file into memory in 1kb chunks and then readString pulls it from memory into other memory?

Comment 2

[(dormant account)]

First one is a conversionstream. It seems to act as a buffering input stream. I am not sure why stuff is being converted here.

So the buffering stream reads in 1k chunks, but the consumer stream is reading in 4k chunks, so every consumer read call, ends up in 4 read syscalls.  Easy fix is to just boost the buffers to a high enough number, but i'd rather have the code figureout number of bytes avail and read in exact amount(ie going from 260 reads() in the case of 260K file, to 1).

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I can take this. Looks like it should be easy.

Should I just make the change in the buffer stream or should both streams read the full length. Are there potential issues if that happens? I've heard of very large (>50MB) sessionstore.js files in extreme atypical cases. Will that cause problems?

Comment 4

[Dietrich Ayala (:dietrich)]

50mb sessionstore.js will cause all kinds of problems outside of just reading the file. i think you should read the whole thing for this bug, and file a followup to cap how much we'll read in.

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

Per discussion with Taras, we should be fine getting rid of the while loop since stream.available() will max out at PR_UINT_32_MAX, but that would mean we'd be trying to read a 4gb+ file. I don't think we need to guard against that case because... well, a 4gb sessionstore.js file?!

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 415488 [details] [diff] [review]
Patch v0.1

cancelling, reviewed on irc

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.2

Added a max file size (100MB) and an error message if we go over it.

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 415685 [details] [diff] [review]
Patch v0.2

r=me assuming you've tested the cap code, and verified that a new session is started and the error makes it to the console.

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #8)
> r=me assuming you've tested the cap code, and verified that a new session is
> started and the error makes it to the console.

I did. Of course it requires that javascript.options.showInConsole is true, but that's consistent with most errors (and in particular, other sessionstore errors).

Comment 10

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/72cff3ab791e

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 415685 [details] [diff] [review]
Patch v0.2

>+      if (fileSize > MAX_FILE_SIZE)
>+        throw "SessionStartup: sessionstore.js was not processed because it was too large.";
Shouldn't you be throwing a Components.Exception object here and pass Cr.NS_ERROR_FILE_TOO_BIG for it?

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #11)
> (From update of attachment 415685 [details] [diff] [review])
> >+      if (fileSize > MAX_FILE_SIZE)
> >+        throw "SessionStartup: sessionstore.js was not processed because it was too large.";
> Shouldn't you be throwing a Components.Exception object here and pass
> Cr.NS_ERROR_FILE_TOO_BIG for it?

I was looking for an existing Cr to throw, but none seemed appropriate.

From https://developer.mozilla.org/en/Table_Of_Errors - NS_ERROR_FILE_TOO_BIG "This error occurs when the createUnique  method cannot create a file with a unique filename. This indicates that too many files have been created already."

Regardless, all we do is report the error to the console, and treat it as if the file didn't exist. The error doesn't get thrown out of this method.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> From https://developer.mozilla.org/en/Table_Of_Errors - NS_ERROR_FILE_TOO_BIG
> "This error occurs when the createUnique  method cannot create a file with a
> unique filename. This indicates that too many files have been created already."
Haha, I bet that description is wrong (I didn't even know we had that page).

Comment 14

[u279076]

Is there anything QA can do to verify this bug?

Comment 15

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #14)
> Is there anything QA can do to verify this bug?

Not unless you want to get dtrace set up & run a script that looks at system read calls.

Comment 16

[u279076]

(In reply to comment #15)
> (In reply to comment #14)
> > Is there anything QA can do to verify this bug?
> 
> Not unless you want to get dtrace set up & run a script that looks at system
> read calls.

Is there any perceptible performance improvement expected that QA can check for?

Comment 17

[(dormant account)]

(In reply to comment #16)

> Is there any perceptible performance improvement expected that QA can check
> for?

No. The overhead can only be meaningfully measured either with good instrumentation or with crappy instrumentation on slow operating systems such as windows mobile.

Comment 18

[u279076]

(In reply to comment #17)
> (In reply to comment #16)
> 
> > Is there any perceptible performance improvement expected that QA can check
> > for?
> 
> No. The overhead can only be meaningfully measured either with good
> instrumentation or with crappy instrumentation on slow operating systems such
> as windows mobile.

Ok, marking as VERIFIED based on faith and the confidence of the developers :)