Last Comment Bug 794234 - Get more information on WRITE_ERROR_SHARING_VIOLATION errors (error 36)
: Get more information on WRITE_ERROR_SHARING_VIOLATION errors (error 36)
Status: RESOLVED FIXED
[mentor=bbondy][lang=c++]
:
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla19
Assigned To: Adri Hilviu
:
Mentors:
Depends on:
Blocks: 794160 715746
  Show dependency treegraph
 
Reported: 2012-09-25 14:07 PDT by Brian R. Bondy [:bbondy]
Modified: 2012-10-13 16:41 PDT (History)
4 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Get more information on WRITE_ERROR_SHARING_VIOLATION errors (error 36) (7.32 KB, patch)
2012-09-25 19:38 PDT, Luqman Aden [:Luqman]
no flags Details | Diff | Splinter Review
Get more information on WRITE_ERROR_SHARING_VIOLATION errors (error 36) (8.09 KB, patch)
2012-10-11 17:35 PDT, Adri Hilviu
netzen: review+
Details | Diff | Splinter Review

Description Brian R. Bondy [:bbondy] 2012-09-25 14:07:26 PDT
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] 2012-09-25 14:16:32 PDT
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] 2012-09-25 19:38:58 PDT
Created attachment 664761 [details] [diff] [review]
Get more information on WRITE_ERROR_SHARING_VIOLATION errors (error 36)
Comment 3 Brian R. Bondy [:bbondy] 2012-09-25 19:45:21 PDT
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] 2012-09-30 18:44:28 PDT
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] 2012-10-08 06:43:44 PDT
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 2012-10-11 17:35:19 PDT
Created attachment 670630 [details] [diff] [review]
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] 2012-10-11 17:42:22 PDT
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] 2012-10-13 05:50:47 PDT
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] 2012-10-13 16:41:17 PDT
https://hg.mozilla.org/mozilla-central/rev/d0f744425ce0

Note You need to log in before you can comment on or make changes to this bug.