Page loading long paused when safebrowsing files are downloaded

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Safe Browsing
P2
normal
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: david.meylan, Assigned: tnguyen)

Tracking

53 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf-])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
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.

Updated

4 months ago
Component: Untriaged → Safe Browsing
Product: Firefox → Toolkit
Comment hidden (typo)
(Assignee)

Comment 2

4 months ago
I guess that bug 1339050 fixed this bug
(Reporter)

Comment 3

4 months ago
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...
(Assignee)

Comment 4

4 months ago
(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
(Assignee)

Comment 6

4 months ago
(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
(Assignee)

Comment 7

4 months ago
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)
(Assignee)

Updated

4 months ago
Flags: needinfo?(gpascutto)
(Assignee)

Comment 8

4 months ago
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)

Updated

3 months ago
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)
(Assignee)

Comment 11

3 months ago
(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-]
Comment hidden (mozreview-request)
(Assignee)

Comment 13

3 months ago
(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 hidden (mozreview-request)

Comment 15

3 months ago
mozreview-review
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 16

3 months ago
mozreview-review
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+
(Assignee)

Updated

3 months ago
Attachment #8869322 - Flags: review?(mcmanus)
(Assignee)

Comment 17

3 months ago
Hi Pattrick,
Could you please review this (in comment 15)?

Comment 18

3 months ago
mozreview-review
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+
(Assignee)

Comment 19

3 months ago
mozreview-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
Comment hidden (mozreview-request)
(Assignee)

Comment 21

3 months ago
Try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8fdd26d8e3a669889e46eca801f71124034aadf
(Assignee)

Updated

3 months ago
Keywords: checkin-needed

Comment 22

3 months ago
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

Comment 23

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8f2b4efc5f0d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.