Closed Bug 762032 Opened 12 years ago Closed 11 years ago

Intermittent test_0201_app_launch_apply_update_svc.js | WindowsError: [Error 13] The process cannot access the file because it is being used by another process: '...\\ExecutableDir.tmp\\bin\\uninstall\\helper.exe'

Categories

(Toolkit :: Application Update, defect)

Product:
Toolkit
Component:
Application Update
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla21
Tracking Flags:
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
firefox21 --- fixed
firefox-esr17 --- fixed

People

(Reporter: emorley, Assigned: robert.strong.bugs)

References

Details

(Keywords: intermittent-failure, Whiteboard: [test which aborts the suite])

Attachments

(3 files, 5 obsolete files)

Patch (v1)
856 bytes, patch
robert.strong.bugs
: review+
lsblakk
: approval-mozilla-aurora+
Details | Diff | Splinter Review
patch rev4
12.42 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
fix it patch rev2
5.28 KB, patch
bbondy
: review+
Details | Diff | Splinter Review 
Rev3 WINNT 6.1 mozilla-inbound opt test xpcshell on 2012-06-05 07:29:49 PDT for push 8dd2e853993e

slave: talos-r3-w7-032

https://tbpl.mozilla.org/php/getParsedLog.php?id=12383931&tree=Mozilla-Inbound

{
TEST-INFO | c:\talos-slave\test\build\xpcshell\tests\toolkit\mozapps\update\test\unit\test_0115_general.js | running test ...
TEST-PASS | c:\talos-slave\test\build\xpcshell\tests\toolkit\mozapps\update\test\unit\test_0115_general.js | test passed (time: 1479.000ms)
TEST-INFO | c:\talos-slave\test\build\xpcshell\tests\toolkit\mozapps\update\test\unit\test_0200_app_launch_apply_update.js | running test ...
TEST-PASS | c:\talos-slave\test\build\xpcshell\tests\toolkit\mozapps\update\test\unit\test_0200_app_launch_apply_update.js | test passed (time: 1869.000ms)
TEST-INFO | c:\talos-slave\test\build\xpcshell\tests\toolkit\mozapps\update\test\unit\test_0201_app_launch_apply_update.js | running test ...
TEST-PASS | c:\talos-slave\test\build\xpcshell\tests\toolkit\mozapps\update\test\unit\test_0201_app_launch_apply_update.js | test passed (time: 7730.000ms)
Traceback (most recent call last):
  File "xpcshell/runxpcshelltests.py", line 915, in <module>
    main()
  File "xpcshell/runxpcshelltests.py", line 911, in main
    if not xpcsh.runTests(args[0], testdirs=args[1:], **options.__dict__):
  File "xpcshell/runxpcshelltests.py", line 795, in runTests
    self.removeDir(self.profileDir)
  File "xpcshell/runxpcshelltests.py", line 321, in removeDir
    shutil.rmtree(dirname)
  File "c:\mozilla-build\python25\Lib\shutil.py", line 169, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "c:\mozilla-build\python25\Lib\shutil.py", line 169, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "c:\mozilla-build\python25\Lib\shutil.py", line 169, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "c:\mozilla-build\python25\Lib\shutil.py", line 174, in rmtree
    onerror(os.remove, fullname, sys.exc_info())
  File "c:\mozilla-build\python25\Lib\shutil.py", line 172, in rmtree
    os.remove(fullname)
WindowsError: [Error 13] The process cannot access the file because it is being used by another process: 'c:\\users\\cltbld\\appdata\\local\\temp\\tmpnvfhv6\\ExecutableDir.tmp\\bin\\uninstall\\helper.exe'
program finished with exit code 1
elapsedTime=2352.151000
TinderboxPrint: xpcshell<br/><em class="testfail">T-FAIL</em>
Unknown Error: command finished with exit code: 1
}
Attached patch Patch (v1) — — Splinter Review 
We should probably increase the amount of time we wait for the helper app to finish running in our tests.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #631893 - Flags: review?(robert.bugzilla)
Blocks: 760577
Blocks: 757632
Attachment #631893 - Flags: review?(robert.bugzilla) → review+
Do you think it makes sense to increase it more or possibly make it more deterministic?
(In reply to Robert Strong [:rstrong] (do not email) from comment #7)
> Do you think it makes sense to increase it more or possibly make it more
> deterministic?

We should probably wait to see if we still see test failures before increasing it more.  As to making it more deterministic, I'm all for it, but I'm afraid I don't know how to do that...
Comment on attachment 631893 [details] [diff] [review]
Patch (v1)

[Approval Request Comment]
This is a test fix which is very useful to have on Aurora.
Attachment #631893 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/6a9d6a79440f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 631893 [details] [diff] [review]
Patch (v1)

[Triage Comment]
Always happy to see some orange get stamped out :) go for it.
Attachment #631893 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://tbpl.mozilla.org/php/getParsedLog.php?id=13481002&tree=Ionmonkey
Summary: Intermittent test_0201_app_launch_apply_update.js | WindowsError: [Error 13] The process cannot access the file because it is being used by another process: '...\\appdata\\local\\temp\\tmpnvfhv6\\ExecutableDir.tmp\\bin\\uninstall\\helper.exe' → Intermittent test_0201_app_launch_apply_update.js, test_0203_app_launch_apply_update_svc.js | WindowsError: [Error 13] The process cannot access the file because it is being used by another process: '...helper.exe'
https://tbpl.mozilla.org/php/getParsedLog.php?id=13551511&tree=Firefox
Whiteboard: [orange] → [orange][test which aborts the suite]
https://tbpl.mozilla.org/php/getParsedLog.php?id=13655483&tree=Mozilla-Inbound
Summary: Intermittent test_0201_app_launch_apply_update.js, test_0203_app_launch_apply_update_svc.js | WindowsError: [Error 13] The process cannot access the file because it is being used by another process: '...helper.exe' → Intermittent test_0201_app_launch_apply_update.js test_0201_app_launch_apply_update_svc.js test_0203_app_launch_apply_update_svc.js | WindowsError: [Error 13] The process cannot access the file because it is being used by another process: '...helper.exe'
Depends on: 793855
https://tbpl.mozilla.org/php/getParsedLog.php?id=15512220&tree=Mozilla-Inbound

{
== BloatView: ALL (cumulative) LEAK STATISTICS, default process 1732

     |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
                                              Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
   0 TOTAL                                          22      216    16132        5 (  447.41 +/-   639.87)    45919        2 ( 1478.95 +/-  2427.03)
  42 Mutex                                          12       12       68        1 (   34.04 +/-    19.25)        0        0 (    0.00 +/-     0.00)
  52 ReentrantMonitor                               16       16       21        1 (    8.12 +/-     4.26)        0        0 (    0.00 +/-     0.00)
 198 nsTArray_base                                   4        4     2231        1 (  259.87 +/-    67.87)        0        0 (    0.00 +/-     0.00)
 199 nsThread                                      112      112        7        1 (    3.77 +/-     1.96)      109        1 (   23.67 +/-     9.26)
 202 nsTimerImpl                                    72       72       18        1 (    5.49 +/-     2.87)      132        1 (   11.98 +/-     5.32)

nsTraceRefcntImpl::DumpStatistics: 244 entries
Traceback (most recent call last):
  File "xpcshell/runxpcshelltests.py", line 992, in <module>
    main()
  File "xpcshell/runxpcshelltests.py", line 988, in main
    if not xpcsh.runTests(args[0], testdirs=args[1:], **options.__dict__):
  File "xpcshell/runxpcshelltests.py", line 866, in runTests
    self.removeDir(self.profileDir)
  File "xpcshell/runxpcshelltests.py", line 325, in removeDir
    shutil.rmtree(dirname)
  File "c:\mozilla-build\python25\Lib\shutil.py", line 169, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "c:\mozilla-build\python25\Lib\shutil.py", line 169, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "c:\mozilla-build\python25\Lib\shutil.py", line 169, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "c:\mozilla-build\python25\Lib\shutil.py", line 174, in rmtree
    onerror(os.remove, fullname, sys.exc_info())
  File "c:\mozilla-build\python25\Lib\shutil.py", line 172, in rmtree
    os.remove(fullname)
WindowsError: [Error 13] The process cannot access the file because it is being used by another process: 'c:\\users\\cltbld\\appdata\\local\\temp\\tmpteufwi\\ExecutableDir.tmp\\bin\\uninstall\\helper.exe'
program finished with exit code 1
}
Depends on: 799532
Depends on: 809071
(In reply to TinderboxPushlog Robot from comment #112)
> edmorley
> https://tbpl.mozilla.org/php/getParsedLog.php?id=16959208&tree=Mozilla-
> Inbound
> Rev3 WINNT 6.1 mozilla-inbound opt test xpcshell on 2012-11-12 07:14:10
> slave: talos-r3-w7-014
> 
> TEST-UNEXPECTED-FAIL |
> c:\talos-
> slave\test\build\xpcshell\tests\toolkit\mozapps\update\test_svc\unit\test_020
> 1_app_launch_apply_update_svc.js | Failed to clean up the test profile
> directory: [Error 13] The process cannot access the file because it is being
> used by another process:
> 'c:\\users\\cltbld\\appdata\\local\\temp\\tmppyy2dj\\ExecutableDir.
> tmp\\bin\\uninstall\\helper.exe'

This occurred after bug 809071 landed.
Whiteboard: [orange][test which aborts the suite] → [test which aborts the suite]
Attached patch don't run helper.exe (obsolete) — — Splinter Review 
As with other tests we shouldn't run helper.exe.

Try run in progress
https://tbpl.mozilla.org/?tree=Try&rev=70802e9d5a1f
Assignee: ehsan → robert.bugzilla
Status: REOPENED → ASSIGNED
Attachment #690676 - Flags: review?(netzen)
Comment on attachment 690676 [details] [diff] [review]
don't run helper.exe

I want to clean this up a bit more
Attachment #690676 - Flags: review?(netzen)
Try run looks good so far
https://tbpl.mozilla.org/?tree=Try&rev=ac4e1db8396b
Attachment #691024 - Attachment is obsolete: true
Attachment #691089 - Flags: review?(netzen)
Comment on attachment 691089 [details] [diff] [review]
don't run helper.exe v3 - better comments

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

This avoids the problem by not running post update at all. Effectively disabling part of the test that is causing problems.

There's a couple reasons I'm not a fan of this approach:
If there is ever a bug in running a post update, after this patch, we wouldn't see it in the tests failing anymore. 
If there is ever a bug in some kind of modal popup in PostUpdate being introduced, we would also not see it after this patch.

Instead (in a different bug) I think we should be adding a test to make sure PostUpdate is actually run when it should be. 
Such a test wouldn't be possible if we took in this patch.

For this bug, perhaps adding something like this to wait for helper.exe instead for both the service and non service cases?
http://dxr.mozilla.org/mozilla-central/toolkit/mozapps/update/test/unit/head_update.js.in.html#l708
Attachment #691089 - Flags: review?(netzen)
(In reply to Brian R. Bondy [:bbondy] from comment #143)
> Comment on attachment 691089 [details] [diff] [review]
> don't run helper.exe v3 - better comments
> 
> Review of attachment 691089 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This avoids the problem by not running post update at all. Effectively
> disabling part of the test that is causing problems.
What specifically is tested when post update runs? Nothing as far as I can tell and as a matter of fact the test is oblivious to post update.

When I added the first tests for the updater way back when the build systems were not to be modified and post update modifies the registry. I have been told in passing that we can do so now but that needs to be verified.

> 
> There's a couple reasons I'm not a fan of this approach:
> If there is ever a bug in running a post update, after this patch, we
> wouldn't see it in the tests failing anymore. 
> If there is ever a bug in some kind of modal popup in PostUpdate being
> introduced, we would also not see it after this patch.
That is not what this test was written to test and since it isn't written for it doesn't handle the case where it is currently causing oranges.

It also didn't catch the bug where the post update exe was showing a UI a short while back.

> Instead (in a different bug) I think we should be adding a test to make sure
> PostUpdate is actually run when it should be. 
> Such a test wouldn't be possible if we took in this patch.
That is not true whatsoever. When I added the xpcshell tests I made sure that they were very focused on the actual thing they are trying to test. I agree this should be tested in xpcshell but there is nothing preventing us from adding a new xpcshell test that actually tests this whereas this test doesn't actually test this functionality... not only on Windows but also on Mac OS X where post update is also available. Such a test should just verify that just the updater performs correctly. Another test outside of the update tests should be added (likely not under update) to verify that if a post update exe is defined that the post update exe does the right thing.
Depends on: 820933
Depends on: 820934
Attached patch patch rev4 — — Splinter Review 
This prevents the running of helper.exe everywhere except for test_0201_app_launch_apply_update_svc.js which will lessen the number of oranges while still addressing the immediately addressable concern raised in comment #143. After bug 820933 and bug 820934 are fixed test_0201_app_launch_apply_update_svc.js can have the same fix as the rest of the tests.
Attachment #691089 - Attachment is obsolete: true
Attachment #691434 - Flags: review?(netzen)
Note: one of the things I didn't like about the service and staging tests is that I am fairly certain they test the same code paths that are already tested by other tests which is not necessary but I didn't want perfect to be the enemy of good to get in the way of those landing. It would be a good thing to try to clean all of these up but there is higher priority work atm.
> What specifically is tested when post update runs? Nothing as far as I can tell and as a 

Just an example but let's say inside the function LaunchWinPostProcess there was a bug that caused a crash.  As of this new patch that crash wouldn't be seen by us.  Yup agreed that we can do another test. 

> and as a matter of fact the test is oblivious to post update.

I didn't claim anything tested if PostUpdate was run, and in particular I did later comment that I would like to see that tested in the future but not as part of this bug, and not as part of this test is OK by me. 

> That is not what this test was written to test and since it isn't written for 
> it doesn't handle the case where it is currently causing oranges.

Nevertheless I believe this patch does reduce the coverage of the code that is run during updates.  Failures often show up even if it's not for the exact thing a test was designed for.  Thanks for posting bug 820934 which would re-introduce this code coverage.

> It also didn't catch the bug where the post update exe was showing a 
> UI a short while back.

I believe that bug only showed up when there were some special files in the installation dir that were being put there by an unrelated installer bug.  There will always be cases that are outside of what our tests cover, but we would still be reducing what is tested with this patch in general.

> That is not true whatsoever. When I added the xpcshell tests I made sure that they were very
>  focused on the actual thing they are trying to test. I agree this should be tested in
>  xpcshell but there is nothing preventing us from adding a new xpcshell test that actually 
> tests this whereas this test doesn't actually test this functionality... not only on Windows 
> but also on Mac OS X where post update is also available. Such a test should just verify 
> that just the updater performs correctly. Another test outside of the update tests should be 
> added (likely not under update) to verify that if a post update exe is defined that the post 
> update exe does the right thing.

Yes that's true.  If we added 2 such tests that actually run PostUpdate though, we may run into this same intermittent problem as this.


> Note: one of the things I didn't like about the service and staging tests is that I am
> fairly certain they test the same code paths that are already tested by other tests which 
> is not necessary 

I agree with making smaller more focused unit tests are better.  I didn't write those tests by the way but am very thankful to the person that did.  I did do quite a bit of intermittent fixes on them though. 

> but I didn't want perfect to be the enemy of good to get in the way of 
> those landing. 

I'm very aware of this principle and agree with it wholeheartedly. 

> It would be a good thing to try to clean all of these up but there is higher 
> priority work atm.

Agreed.

----

Summary: 

It is a valid concern that there will be less of the code covered, even if the tests were not designed to test that specifically.  And that's why I brought it up. 

If you're OK with the temporary loss of code coverage until bug 820934 lands, you can go ahead and r=me with rev3.  
The patch looks good otherwise.  

Please go with patch v4 if you would like to still have coverage of the code I mentioned above though.
Comment on attachment 691434 [details] [diff] [review]
patch rev4

r=me with the preferred patch as per Comment 147
Attachment #691434 - Flags: review?(netzen) → review+
I'm not sure I understand. With this patch we still launch the post update process but instead of launching it (in the same way) across 4 tests it is only launched once. How does this reduce the code coverage? As I see it, it is just running it once instead of 4 times.
The initial comments were in relation to patch v3 which existed at the time of your first reply without v4 existing.  See the summary section for clarification vs v4 though.
Cool and thanks!
patch rev 4 pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/049f91872fbf

With this patch it should now only fail on test_0201_app_launch_apply_update_svc.js

Please leave this bug open
Whiteboard: [test which aborts the suite] → [test which aborts the suite][leave open]
Summary: Intermittent test_0201_app_launch_apply_update.js test_0201_app_launch_apply_update_svc.js test_0203_app_launch_apply_update_svc.js | WindowsError: [Error 13] The process cannot access the file because it is being used by another process: '...helper.exe' → Intermittent test_0201_app_launch_apply_update_svc.js | WindowsError: [Error 13] The process cannot access the file because it is being used by another process: '...helper.exe'
Summary: Intermittent test_0201_app_launch_apply_update_svc.js | WindowsError: [Error 13] The process cannot access the file because it is being used by another process: '...helper.exe' → Intermittent test_0201_app_launch_apply_update_svc.js | WindowsError: [Error 13] The process cannot access the file because it is being used by another process: '...\\ExecutableDir.tmp\\bin\\uninstall\\helper.exe'
I think this should be enough. It is safe to delete the helper.exe in this instance since it is deleting the copy of helper.exe and not the one in dist/bin/uninstall
Attachment #709414 - Flags: review?(netzen)
Comment on attachment 709414 [details] [diff] [review]
patch - wait until it is possible to remove helper.exe

I'm going to go with a more generic approach that will hopefully also fix bug 823996
Attachment #709414 - Attachment is obsolete: true
Attachment #709414 - Flags: review?(netzen)
Attached patch patch (obsolete) — — Splinter Review 
This will make it simple to add maintenanceservice_installer.exe to fix bug 823996.
Attachment #709423 - Flags: review?(netzen)

        Attachment #709423 -
        Attachment is obsolete: true

        Attachment #709423 -
        Flags: review?(netzen)

        Attachment #709424 -
        Flags: review?(netzen)

    
    

    
      
  

    
    

    
  
Comment on attachment 709424 [details] [diff] [review]
fix it patch rev2

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

::: toolkit/mozapps/update/test/unit/head_update.js.in
@@ +1561,5 @@
> +          fileBak.remove(false);
> +        }
> +        file.copyTo(fileBak.parent, fileBak.leafName);
> +        file.remove(false);
> +        fileBak.moveTo(file.parent, file.leafName);

We could accomplish this without the copyTo via CreateFile but using js ctype is not worth the effort here.

        Attachment #709424 -
        Flags: review?(netzen) → review+

    
    

    
  
Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/3196b34c41e2

I don't *think* (fingers crossed) that this needs to be left open after this patch lands on mozilla-central
Whiteboard: [test which aborts the suite][leave open] → [test which aborts the suite]
Target Milestone: mozilla16 → mozilla21

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/3196b34c41e2
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 11 years ago
Resolution: --- → FIXED

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: