409815 - Download manager virus scan API call should time out after a while

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect, P3)

Description

[Shawn Wilsher :sdwilsh]

We want the windows api call to just be dropped after a short while.  The idea is that after a minute we just assume everything is OK and set the download to a complete state.  After two minutes, we just drop the thread handling the request (but if something comes in during that time, we handle it accordingly).

Comment 1

[Rob Arnold [:robarnold]]

Attachment: Virus scanner that hangs on initialization

Running this program before starting up the browser will hang the scanning thread and thus the download. Killing the program (Ctrl+C) will let the download continue with a warning in the console.

Comment 2

[Rob Arnold [:robarnold]]

Attachment: Proposed solution v1

Spawns a watchdog thread to watch scans. Watchdog timeout period is 60 seconds. Scans that complete after the timeout are ignored.

Comment 3

[Rob Arnold [:robarnold]]

Comment on attachment 295539 [details]
Virus scanner that hangs on initialization

After rebooting my laptop, it seems that my av scanner no longer hangs on initialization, but just fails instantly.

Comment 4

[Jim Mathies [:jimm]]

Just out of curiosity, how bad is the hanging problem? Is it specific to a particular vendor maybe?

Comment 5

[Shawn Wilsher :sdwilsh]

It seems like older computers, and there was one vendor in particular.  See Bug 412204 for more details.

Comment 6

[Jim Mathies [:jimm]]

Hey Rob, just and FYI this patch was broken by the checkin from bug 393305. I'll try to peice it together locally to test.

Comment 7

[Jim Mathies [:jimm]]

Been running this for a day or so now, looks to be working well. We need an updated patch though to address the bustage before I can r+.

Comment 8

[Rob Arnold [:robarnold]]

Attachment: unbitrotted patch

The behavior of regular scans should not have changed since the last patch.

Comment 9

[Rob Arnold [:robarnold]]

Attachment: Virus scanner that hangs on initialization

Since the download manager looks for scanners at startup and not at scan time, you must run this program before you run firefox. While it is running, firefox (or even the test scanner in bug 103487) will hang when it tries to initialize the scanner. Killing the program (nicely as it asks) will allow any current and future calls to return w/an error. You will see firefox print out a warning on the console when that happens.

Comment 10

[Jim Mathies [:jimm]]

I'm going to resist dinging this on line lengths, as I think the 80 char limit is a bit outdated. But someone else might ding you on it, so you might want to shorten some of those up. :)

Ran this today, no problems. The code looks good. r+

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

Do we need an SR to move this forward?  Or just approval?  Some folks who are really seeing issues (myself included, at times), especially with AVG Free, would like to see this in the nightlies, so we can further test.

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 298807 [details] [diff] [review]
unbitrotted patch

Edward or I need to review this before it can land.

Comment 13

[Jim Mathies [:jimm]]

Someone should probably look at threading stuff as it relates to mozilla apps - I look at the win code but there's some functionality in here I'm not qualified to approve (yet :).

Comment 14

[Shawn Wilsher :sdwilsh]

Comment on attachment 298807 [details] [diff] [review]
unbitrotted patch

>+// This destructor appeases the compiler; it would otherwise complain about an incomplete
>+// type for nsDownloadWatchdog in the instantiation of nsAutoPtr::~nsAutoPtr
>+//
>+// Plus, it's a handy location to call nsDownloadScannerWatchdog::Shutdown from
nit: end of sentence grammar please

>+  // Don't release upon timeout since the scanning thread is still live and references member variables
nit: comment line wrapping at 80 chars please.

>+PRBool
>+nsDownloadScanner::Scan::SetState(AVScanState newState, AVScanState expectedState) {
>+  PRBool gotExpectedState = PR_FALSE;
>+  EnterCriticalSection(&mStateSync);
>+  if(gotExpectedState = (mStatus == expectedState))
>+    mStatus = newState;
>+  LeaveCriticalSection(&mStateSync);
>+  return gotExpectedState;
>+}
JavaDoc style comment on this?  I'm not sure I understand why it's doing what it's doing...

>+nsDownloadScannerWatchdog::nsDownloadScannerWatchdog() 
>+  : mNewItemEvent(NULL), mQuitEvent(NULL) {
>+  InitializeCriticalSection(&mQueueSync);
>+}
Can InitializeCriticalSection fail?


>+    PRBool SetState(AVScanState newState, AVScanState expectedState);
Although previously mentioned javadoc comment should in the .h file here.

r=sdwilsh

Thanks a ton!

Comment 15

[Shawn Wilsher :sdwilsh]

rob - it's my understanding that you want to land this after the IAttachmentExecute stuff, and this timeout will only be for IOfficeAttachment, or both? 

Comment 16

[Rob Arnold [:robarnold]]

It will work for both (the entire thread's execution actually).

Comment 17

[Rob Arnold [:robarnold]]

Attachment: Unbitrotted patch

Changes since the last patch:
Scan::Run is invoked only once on behalf of the watchdog or the Scan itself
If the Scan cannot be initialized, it is not handed off to the watchdog

The biggest change of this patch is that Scan::mStatus is now written to by multiple threads; getting/setting this requires entering the mStateSync critical section or calling SetState which performs a compare & swap on it. This allows the scanning thread and watchdog threads to avoid trashing each others values, and to avoid extra work by discovering that the other has been executing.

Comment 18

[Rob Arnold [:robarnold]]

Attachment: Forgot to rediff

I forgot to rediff after a minor change

Comment 19

[Jim Mathies [:jimm]]

starting to look at this today...

Comment 20

[Jim Mathies [:jimm]]

+    mWatchdog = new nsDownloadScannerWatchdog();
+    rv = mWatchdog->Init();
...
+    // We timed out, so just release
+    ReleaseDispatcher* releaser = new ReleaseDispatcher(this);
+    NS_ADDREF(releaser);
+    NS_DispatchToMainThread(releaser);
...
+    ReleaseDispatcher* releaser = new ReleaseDispatcher(scan);
+    NS_ADDREF(releaser);

We should check the results here for failure.

+  while (0 != queueItemsLeft ||
+         (WAIT_OBJECT_0 + 1) != (waitStatus = WaitForMultipleObjects(2, waitHandles, FALSE, INFINITE)) &&
+         waitStatus != WAIT_FAILED) {

nit - line length

+    scan = reinterpret_cast<Scan*>(watchdog->mScanQueue.Pop());

Is there any chance this could return a null?

In general the code looks good. Some detailed testing with hanging scanners didn't bring to light any issues.

Comment 21

[Shawn Wilsher :sdwilsh]

Comment on attachment 303445 [details] [diff] [review]
Forgot to rediff

>+ * Scan object will dispatch its run method to the main thread; this will
>+ * release the watchdog thread's addref on the Scan. Note that
Note that what?

I take it windows uses "events" instead of "signals"?  (I'm familiar with pthreads, not windows threads).

>+    if (FAILED(rv)) {
>+      mWatchdog = nsnull;
>+    }
nit: no bracing

s/SetState/SetAndCheckState/g ?

r=sdwilsh

Comment 22

[Rob Arnold [:robarnold]]

Attachment: Updated to address comments

(In reply to comment #20)
> +    mWatchdog = new nsDownloadScannerWatchdog();
> +    rv = mWatchdog->Init();
> ...
> +    // We timed out, so just release
> +    ReleaseDispatcher* releaser = new ReleaseDispatcher(this);
> +    NS_ADDREF(releaser);
> +    NS_DispatchToMainThread(releaser);
> ...
> +    ReleaseDispatcher* releaser = new ReleaseDispatcher(scan);
> +    NS_ADDREF(releaser);
> 
> We should check the results here for failure.

Fixed. In the case of the releasers, we will leak memory but I wasn't sure if printing a warning would be appropriate.

> 
> +  while (0 != queueItemsLeft ||
> +         (WAIT_OBJECT_0 + 1) != (waitStatus = WaitForMultipleObjects(2,
> waitHandles, FALSE, INFINITE)) &&
> +         waitStatus != WAIT_FAILED) {
> 
> nit - line length

Fixed, but the line breaks look odd now.

> 
> +    scan = reinterpret_cast<Scan*>(watchdog->mScanQueue.Pop());
> 
> Is there any chance this could return a null?

No. The watchdog thread is the only consumer of the queue. So from the loop precondition we know that one of the following holds:
a) There were more items left on the queue last time we checked
b) waitStatus == WAIT_OBJECT_0 because it is not WAIT_OBJECT_1, WAIT_FAILED or WAIT_TIMEOUT (infinite wait time) so the mNewEvent Event was signaled which means that the a new Scan was pushed onto a previously empty queue (empty when the main thread checked meaning that the watchdog thread is somewhere between LeaveCriticalSection and the WaitForMultipleObjects in the loop and the watchdog knows that the queue is empty too). Thus the watchdog thread will Wait on mNewEvent which is signaled so it returns immediately and the watchdog thread proceeds to pop an element off a non-empty queue.

> 
> In general the code looks good. Some detailed testing with hanging scanners
> didn't bring to light any issues.
> 
Excellent. Some QA on this wouldn't hurt either.

(In reply to comment #21)
> (From update of attachment 303445 [details] [diff] [review])
> >+ * Scan object will dispatch its run method to the main thread; this will
> >+ * release the watchdog thread's addref on the Scan. Note that
> Note that what?

Hmm, either I didn't finish writing that comment or else it got lost in my Mercurial adventure. I wrote something that seemed appropriate.

> 
> I take it windows uses "events" instead of "signals"?  (I'm familiar with
> pthreads, not windows threads).

Yes, Events are signaled or non signaled. Waits on events block until the event is signaled. mNewEvent and mQuitEvent are both auto-reset events so after the Wait finishes, they are reset to the non-signaled state.

> 
> >+    if (FAILED(rv)) {
> >+      mWatchdog = nsnull;
> >+    }
> nit: no bracing

Fixed.

> 
> s/SetState/SetAndCheckState/g ?
> 

%s/SetState/CheckAndSetState/g

Comment 23

[Shawn Wilsher :sdwilsh]

Comment on attachment 304878 [details] [diff] [review]
Updated to address comments

this already has review

Comment 24

[Rob Arnold [:robarnold]]

Checking in toolkit/components/downloads/src/nsDownloadScanner.cpp;[e-mail]:/cvsroot ci -toolkit
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadScanner.cpp,v  <--  nsDownloadScanner.cpplkit
new revision: 1.13; previous revision: 1.12
done
Checking in toolkit/components/downloads/src/nsDownloadScanner.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadScanner.h,v  <--  nsDownloadScanner.h
new revision: 1.8; previous revision: 1.7
done