794234 - Get more information on WRITE_ERROR_SHARING_VIOLATION errors (error 36)

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

I would like to split WRITE_ERROR_SHARING_VIOLATION into 3 errors.  

It would still only get reported in the same case as before from this loop:

> const int max_retries = 10;
> int retries = 1;
> DWORD lastWriteError = 0;
> do {
>   callbackFile = CreateFileW(targetPath,
>                              DELETE | GENERIC_WRITE,
>                              // allow delete, rename, and write
>                              FILE_SHARE_DELETE | FILE_SHARE_WRITE,
>                              NULL, OPEN_EXISTING, 0, NULL);
> ...
>  } else if (ERROR_SHARING_VIOLATION == lastWriteError) {
>    WriteStatusFile(WRITE_ERROR_SHARING_VIOLATION);// *******HERE!!!!
> } else {

The line above labeled*******HERE!!!! would be split up into 3 depending on each of these 3 cases:

> if (pid > 0) {
>    HANDLE parent = OpenProcess(SYNCHRONIZE, false, (DWORD) pid);
>    if (parent) {
>      DWORD result = WaitForSingleObject(parent, 5000);
>      CloseHandle(parent);
>      if (result != WAIT_OBJECT_0)
>        return 1;
>      //1 (WRITE_ERROR_SHARING_VIOLATION_SIGNALED)
>    } else {
>      //2 (WRITE_ERROR_SHARING_VIOLATION_NOPROCESSFORPID)
>    }
>  } else {
>    //3 (WRITE_ERROR_SHARING_VIOLATION_NOPID)
>  }

I think it would provide some insight into if the cause of a failed update was because of a different session that had firefox open, or if it was because of a zombie state pid that's sitting around.   Or perhaps it'll uncover some other investigation.

In particular this information would be useful for investigating cases like bug 794160 and for investigating intermittent update xpcshell test failures like bug 715746.

Comment 1

[Brian R. Bondy [:bbondy]]

Mentored bug information:

How this will help Mozilla: 
---------------------------
This bug will help us get more information on a small number of failed updates.
The error codes are sent up to Telemetry already, so adding new error codes will help us get more info. 


Implementation information:
---------------------------

1. The error is currently defined as 36, toolkit\mozapps\update\common\errors.h
We should comment out that error code and produce 3 more errors with values WRITE_ERROR_SHARING_VIOLATION_SIGNALED=44, WRITE_ERROR_SHARING_VIOLATION_NOPROCESSFORPID=45, and WRITE_ERROR_SHARING_VIOLATION_NOPID=46.

2. In Comment 0 I quoted 2 blocks of code from toolkit\mozapps\update\updater\updater.cpp.  You will need to have a variable that stores the information in each of the 3 cases in the second block.  And once it gets to the first block of code, it would report that error instead of WRITE_ERROR_SHARING_VIOLATION.

3. A very tiny bit of JS is also needed for this bug, but all that's needed is to expand lines like this into 3 in toolkit\mozapps\update\nsUpdateService.js:

- update.errorCode == WRITE_ERROR_SHARING_VIOLATION ||
+ update.errorCode == WRITE_ERROR_SHARING_VIOLATION_SIGNALED ||
+ update.errorCode == WRITE_ERROR_SHARING_VIOLATION_NOPROCESSFORPID ||
+ update.errorCode == WRITE_ERROR_SHARING_VIOLATION_NOPID ||

Comment 2

[Luqman Aden [:Luqman]]

Attachment: Get more information on WRITE_ERROR_SHARING_VIOLATION errors (error 36)

Comment 3

[Brian R. Bondy [:bbondy]]

Comment on attachment 664761 [details] [diff] [review]
Get more information on WRITE_ERROR_SHARING_VIOLATION errors (error 36)

Review of attachment 664761 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

::: toolkit/mozapps/update/common/errors.h
@@ +53,5 @@
>  #define SERVICE_INSTALLDIR_ERROR 33
>  
>  #define NO_INSTALLDIR_ERROR 34
>  #define WRITE_ERROR_ACCESS_DENIED 35
> +//#define WRITE_ERROR_SHARING_VIOLATION 36

nit: Please replace this line with:
// #define WRITE_ERROR_SHARING_VIOLATION 36 // Replaced with errors 44-46

::: toolkit/mozapps/update/nsUpdateService.js
@@ +132,5 @@
>  const SERVICE_COULD_NOT_LOCK_UPDATER       = 32;
>  const SERVICE_INSTALLDIR_ERROR             = 33;
>  
> +const WRITE_ERROR_ACCESS_DENIED                     = 35;
> +//const WRITE_ERROR_SHARING_VIOLATION               = 36;

nit: Ditto for this file with the replace with comment and whitespace

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2831,5 @@
>          LogFinish();
>          if (ERROR_ACCESS_DENIED == lastWriteError) {
>            WriteStatusFile(WRITE_ERROR_ACCESS_DENIED);
>          } else if (ERROR_SHARING_VIOLATION == lastWriteError) {
> +          if (pid > 0) {

We'll have to use the block above to get this information and save it to a variable.  Then report it here.  The reason is that the process will already be closed by this time and we need the information before we get here.

Comment 4

[Brian R. Bondy [:bbondy]]

Hi Luqman Aden, do you think you'll have time to implement the review comments on your patch? Otherwise I'll find another owner if not.

Comment 5

[Brian R. Bondy [:bbondy]]

Someone I used to work with (Adri) expressed interest in picking up a bug, so assigning him to take over on this bug.

Comment 6

[Adri Hilviu]

Attachment: Get more information on WRITE_ERROR_SHARING_VIOLATION errors (error 36)

Offset the error numbers by 2 (46-48) since 2 new error messages were introduced after this ticket was created.

Comment 7

[Brian R. Bondy [:bbondy]]

Comment on attachment 670630 [details] [diff] [review]
Get more information on WRITE_ERROR_SHARING_VIOLATION errors (error 36)

Review of attachment 670630 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks for the patch! 
This will help us gather information on some intermittent test errors we've been having.

I'll push this to a test branch called oak before pushing to mozilla-central and test updates there.

Comment 8

[Brian R. Bondy [:bbondy]]

Congratulations on landing your first patch!

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f744425ce0

From here someone will pickup the changeset soon from mozilla-inbound and put it on mozilla-central which is where Nightly builds are built from: http://nightly.mozilla.org/

within 6 weeks after that it goes from Nightly to Aurora, then 6 weeks after that from Aurora to Beta, then 6 weeks after that from Beta to Release.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d0f744425ce0