Closed Bug 920017 Opened 11 years ago Closed 11 years ago

Race condition in BackgroundFileSaver when setTarget is called multiple times in append mode

Categories

(Core :: Networking: File, defect)

Product:
Core
Component:
Networking: File
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla27
Tracking Flags:
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Fixed patch
16.01 KB, patch
mcmanus
: review+
lsblakk
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
The Downloads API tests surfaced a race condition that may happen in
BackgroundFileSaver. In append mode, the first call to setTarget should set
the file to append to, and subsequent calls should allow it to be renamed.
However, if setTarget is called rapidly for two times, the worker thread has
no chance to notice the first call, and we end up appending to the wrong file.
Attached patch The patch (obsolete) — — Splinter Review 

        Attachment #809164 -
        Flags: review?(mcmanus)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fixed patch
        — 
          — Splinter Review
      

    


  
Multithreading is hard, this code is complex, and regression tests are good.

This patch fixes the completion condition, and a similar version passed try:

https://tbpl.mozilla.org/?tree=Try&rev=ce27bf80cec1

        Attachment #809164 -
        Attachment is obsolete: true

        Attachment #809164 -
        Flags: review?(mcmanus)

        Attachment #809752 -
        Flags: review?(mcmanus)

    
  
Blocks: 906817

    
  
Blocks: 919100

        Attachment #809752 -
        Flags: review?(mcmanus) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/c82c6b643f87
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

    
    

    
  
Comment on attachment 809752 [details] [diff] [review]
Fixed patch

We might uplift this to Aurora to fix intermittent test failures.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 887425
User impact if declined: Virtually none, but fixes intermittent test failures.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low, has tests
String or IDL/UUID changes made by this patch: None

        Attachment #809752 -
        Flags: approval-mozilla-aurora?

        Attachment #809752 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: