Closed Bug 1361699 Opened 7 years ago Closed 7 years ago

Page loading long paused when safebrowsing files are downloaded

Categories

(Toolkit :: Safe Browsing, defect, P2)

Product:
Toolkit
Component:
Safe Browsing
Version:
53 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Project Flags:
Performance Impact none
Tracking Flags:
Tracking Status
relnote-firefox --- 60+
firefox-esr52 60+ fixed
firefox55 --- fixed

People

(Reporter: david.meylan, Assigned: tnguyen)

Details

Attachments

(1 file)

Bug 1361699 - Add buffer when writing hashstore to file
59 bytes, text/x-review-board-request
gcp
: review+
mcmanus
: review+
RyanVM
: approval-mozilla-esr52+
Details 
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170418123315

Steps to reproduce:

This happens on Windows.
For some internal reasons, we have stored the Firefox profiles on a network share (using the 'profiles.ini' file).
Every 30 minutes, the web navigation is paused for about 1 minute, and this happens when the safebrowsing files are being downloaded. This happens on any web site.


Actual results:

All the time during the download of the safebrowsing files (i.e. <firefox_profile>/safebrowsing/goog-*.sbstore and .pset) the page loading is paused.

This is noticeable only when the firefox profile is stored on a network share (takes about 1 minute). When the firefox profile is stored locally on the PC, it takes only 2 seconds).

This can be reproduced on will.


Expected results:

The page loading should not be impacted by the safebrowsing files download.
Component: Untriaged → Safe Browsing
Product: Firefox → Toolkit
I guess that bug 1339050 fixed this bug
Great that the safebrowsing db download will be "asynced". So the problem of page loading is fixed...

But, using the sysinternals process monitor tool (i.e. 'procmon.exe'), I have noticed that the safebrowsing DB files download time is way longer when stored on a network share than on a local drive (much more than what it should be): it takes something like 2 seconds on a local drive, but up to 1 minute on the network share (on a gigabit ethernet network). Maybe this could be due to the way the some safebrowsing db files are written (i.e. 'goog-*.sbstore'), namely in small "packs" of 4 blocks... I suspect that this way of writing a file is disadvantageous on a network share.

By the way, is there a variable in Firefox config, that defines the path where the safebrowsing files are stored? I could not find one...
(In reply to david.meylan from comment #3)
> Great that the safebrowsing db download will be "asynced". So the problem of
> page loading is fixed...
> 
> But, using the sysinternals process monitor tool (i.e. 'procmon.exe'), I
> have noticed that the safebrowsing DB files download time is way longer when
> stored on a network share than on a local drive (much more than what it
> should be): it takes something like 2 seconds on a local drive, but up to 1
> minute on the network share (on a gigabit ethernet network). Maybe this
> could be due to the way the some safebrowsing db files are written (i.e.
> 'goog-*.sbstore'), namely in small "packs" of 4 blocks... I suspect that
> this way of writing a file is disadvantageous on a network share.
> 

You are right, we wrote update data every small piece of prefix, probably it causes slower writing here. 
> By the way, is there a variable in Firefox config, that defines the path
> where the safebrowsing files are stored? I could not find one...
I think the safebrowsing writing is hardcoded to profile folder and you have to manually change the profile folder using .ini file
Right now safebrowsing write use nsIOutputStream, so there are lots of 4-bytes writes, maybe we should use something like NS_BufferOutputStream[1]. I guess we don't have this problem when when writing prefix set because it already use BufferedOutputStream[2].

By the way, since V4 no longer has .sbstore files, so if this really happens when write .sbstore files, then maybe we can get rid of this issue after V4 roll out.

[1] https://searchfox.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp#1377
[2] https://searchfox.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp#379
(In reply to Dimi Lee[:dimi][:dlee] from comment #5)
> Right now safebrowsing write use nsIOutputStream, so there are lots of
> 4-bytes writes, maybe we should use something like NS_BufferOutputStream[1].
> I guess we don't have this problem when when writing prefix set because it
> already use BufferedOutputStream[2].

In bug 1356854 we had crash with nsCheckSummedOutputStream, probably it's worth to change to NS_BufferOutputStream
Hi Gcp,
Do you have any concern if we change the usage of nsSafeFileOutputStream to nsIBufferedOutputStream
https://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/toolkit/components/url-classifier/nsCheckSummedOutputStream.h#18
https://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/netwerk/base/nsFileStreams.h#262
I discussed with Dimi and probably nsIBufferedOutputStream is good enough for us.
Besides, I am not quite sure if the change could fix bug 1356854, but I guess at least it would reduce the rate of crashes.
Flags: needinfo?(gpascutto)
Flags: needinfo?(gpascutto)
That said, afaik, the advantage of nsSafeFileOutputStream is to ensure we could write data to a temporary file successfully (then the last step is to move the temporary to target file). That protects against incomplete writes unexpectedly.
Imho, using nsIBufferedOutputStream we could gain the same purpose, that is, we store data to buffer and only write to file once the buffer full (or finish writing). I prefer to use a system call 1024 bytes PR_write than 256 system calls of 4 bytes

Besides, I see, in current design, we temporarily store update data to a folder and then could "swap" between temporarily and backup folder (that could protect us if there're something wrong in writing data, checksum could also help). I think nsIBufferedOutputStream is good enough for us (currently writing PrefixSet is using nsIBufferedOutputStream too)

Hi Francois,
Do you have any concern to change nsSafeFileOutputStream to nsIBufferedOutputStream?
Another approach is that we could create a wrapper of nsSafeFileOutputStream in nsIBufferedOutputStream so that we could use the advantage of nsIBufferedOutputStream interface.
Also ni gcp who could know the history of nsSafeFileOutputStream?
Flags: needinfo?(gpascutto)
Flags: needinfo?(francois)
Whiteboard: [qf]
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #8)
> Do you have any concern to change nsSafeFileOutputStream to
> nsIBufferedOutputStream?
> Another approach is that we could create a wrapper of nsSafeFileOutputStream
> in nsIBufferedOutputStream so that we could use the advantage of
> nsIBufferedOutputStream interface.

That sounds fine to me.
Assignee: nobody → tnguyen
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(francois)
Priority: -- → P2
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #8)
> That protects against incomplete writes unexpectedly.

Right. Unfortunately that doesn't mean you can rely on being able to read the file back in (the disk might have corrupted it). So you could argue the gain is minimal.

> Imho, using nsIBufferedOutputStream we could gain the same purpose, that is,
> we store data to buffer and only write to file once the buffer full (or
> finish writing). I prefer to use a system call 1024 bytes PR_write than 256
> system calls of 4 bytes

I hope you're not intending to allocate a buffer large enough to store the entire file?

The main point to take into account here is to make 200% sure that whatever is reading the now-possibly-even-more-likely-to-be-truncated-file in is extremely robust to dealing with corrupted input data.
Flags: needinfo?(gpascutto)
(In reply to Gian-Carlo Pascutto [:gcp] from comment #10)
> (In reply to Thomas Nguyen[:tnguyen] ni plz from comment #8)
> 
> > Imho, using nsIBufferedOutputStream we could gain the same purpose, that is,
> > we store data to buffer and only write to file once the buffer full (or
> > finish writing). I prefer to use a system call 1024 bytes PR_write than 256
> > system calls of 4 bytes
> 
> I hope you're not intending to allocate a buffer large enough to store the
> entire file?

Ah, I just use the same buffer we are using in prefix set MAX_BUFFER_SIZE = 64 * 1024.

> The main point to take into account here is to make 200% sure that whatever
> is reading the now-possibly-even-more-likely-to-be-truncated-file in is
> extremely robust to dealing with corrupted input data.
Yep, I would keep using the same checksum idea when writing file. We will check the checksum everytime we are about to read a file (if the checksum indicates that the file is corrupted then we should clear the database)
Thanks gcp
Whiteboard: [qf] → [qf-]
(In reply to Gian-Carlo Pascutto [:gcp] from comment #10)
> The main point to take into account here is to make 200% sure that whatever
> is reading the now-possibly-even-more-likely-to-be-truncated-file in is
> extremely robust to dealing with corrupted input data.
Hi Francois
I just added a BufferedOutputStream wrapper, so that we still keep the same safe writing. Could you please take a look?
Comment on attachment 8869322 [details]
Bug 1361699 - Add buffer when writing hashstore to file

https://reviewboard.mozilla.org/r/140866/#review144660

Redirecting this review to gcp who knows more about the storage side of Safe Browsing and the pitfalls of allocating non-trivial amounts of memory in that code.

::: netwerk/base/nsBufferedStreams.h:102
(Diff revision 2)
>      nsCOMPtr<nsIInputStreamCallback> mAsyncWaitCallback;
>  };
>  
>  ////////////////////////////////////////////////////////////////////////////////
>  
> -class nsBufferedOutputStream final : public nsBufferedStream,
> +class nsBufferedOutputStream  : public nsBufferedStream,

That will need to be reviewed by a necko peer.
Attachment #8869322 - Flags: review?(francois)
Attachment #8869322 - Flags: review?(gpascutto)
Comment on attachment 8869322 [details]
Bug 1361699 - Add buffer when writing hashstore to file

https://reviewboard.mozilla.org/r/140866/#review146096
Attachment #8869322 - Flags: review?(gpascutto) → review+
Attachment #8869322 - Flags: review?(mcmanus)
Hi Pattrick,
Could you please review this (in comment 15)?
Comment on attachment 8869322 [details]
Bug 1361699 - Add buffer when writing hashstore to file

https://reviewboard.mozilla.org/r/140866/#review147144

r+ netwerk bit
Attachment #8869322 - Flags: review?(mcmanus) → review+
Comment on attachment 8869322 [details]
Bug 1361699 - Add buffer when writing hashstore to file

https://reviewboard.mozilla.org/r/140864/#review148166

Rebased to m-c
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8f2b4efc5f0d
Add buffer when writing hashstore to file r=gcp,mcmanus
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8f2b4efc5f0d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8869322 [details]
Bug 1361699 - Add buffer when writing hashstore to file

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Excessive I/O at startup (intermittent), see bug 1436297.
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Low, it's not a large patch and it has been in release for many cycles. We've not had to do any follow-up fixes.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8869322 - Flags: approval-mozilla-esr52?
Comment on attachment 8869322 [details]
Bug 1361699 - Add buffer when writing hashstore to file

Taking for ESR 52.8 per the discussion in bug 1436297. Let's hope it helps...
Attachment #8869322 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
We should add a release note for esr52.8.
relnote-firefox: --- → ?
Added to the ESR 52.8.0 relnotes.
relnote-firefox: ? → 60+
Performance Impact: --- → -
Whiteboard: [qf-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: