Closed Bug 1306235 Opened 8 years ago Closed 8 years ago

Intermittent dom/xhr/tests/test_xhr_progressevents.html | lengthComputable while running data for with length for blob[2] - got false, expected true

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: baku)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 2 obsolete files)

xhr.patch
7.80 KB, patch
smaug
: review+
wisniewskit
: feedback+
Details | Diff | Splinter Review
Attached patch xhr.patch (obsolete) — — Splinter Review 
Assignee: nobody → amarchesini

        Attachment #8797138 -
        Flags: review?(wisniewskit)

    
    

    
  
Comment on attachment 8797138 [details] [diff] [review]
xhr.patch

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

I don't think that my r+ is enough to actually push any fixes, but this is indeed the correct fix. Thanks baku!

        Attachment #8797138 -
        Flags: review?(wisniewskit)

        Attachment #8797138 -
        Flags: review?(bugs)

        Attachment #8797138 -
        Flags: review+

        Attachment #8797138 -
        Flags: review?(bugs) → review+

    
    

    
      
  

    
    

    
  
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf588a950083
XHR should set lengthComputable only if total value is set, r=wisniewskit, r=smaug

    
    

    
  
Hmm, strange that I missed that. It seems for us that the fix is really to use:

>init.mLengthComputable = aTotal <= 0;

Because we use -1 as an "unknown size" value in some cases, but not all of them. I'm testing this theory now.

    
    

    
  
> Because we use -1 as an "unknown size" value in some cases, but not all of
> them. I'm testing this theory now.

I think that: some of the Blob/XHR mochitests are wrong. and the WPT should require a check about mFlagAborted.

    
    

    
  
(In reply to Andrea Marchesini [:baku] from comment #11)
> I think that: some of the Blob/XHR mochitests are wrong. and the WPT should
> require a check about mFlagAborted.

Yeah, I think you may be right. Unfortunately I don't think I'll have the time to investigate it for a couple of days, though...

    
    

    
      
  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        xhr.patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8797138 -
        Attachment is obsolete: true

        Attachment #8798102 -
        Flags: review?(bugs)

        Attachment #8798102 -
        Flags: feedback?(wisniewskit)

    
    

    
  
Comment on attachment 8798102 [details] [diff] [review]
xhr.patch

>-  mLoadTotal = mLoadTransferred;
>+  if (mLoadTransferred) {
>+    mLoadTotal = mLoadTransferred;
>+  } else {
>+    mLoadTotal = -1;
>+  }
Ok, I guess this is fine given:
"If length is not 0, set the lengthComputable attribute value to true and the total attribute value to length. "



> 
>-  is(e.lengthComputable, "total" in data, "lengthComputable" + test);
>   if ("total" in data) {
>+    ok(e.lengthComputable, "lengthComputable" + test);
>     is(e.total, data.total, "total" + test);
>   }
I don't understand why this change is needed.  What behavior are we changing here?

    
    

    
  
Comment on attachment 8798102 [details] [diff] [review]
xhr.patch

I think the change to the test is wrong. We end up losing tests for lengthComputable in certain cases.
If lengthComputable is now set in more cases, please explain why, and update the relevant test to have total

        Attachment #8798102 -
        Flags: review?(bugs) → review-

    
    

    
      
  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        xhr.patch
        — 
          — Splinter Review
      

    


  

        Attachment #8798102 -
        Attachment is obsolete: true

        Attachment #8798102 -
        Flags: feedback?(wisniewskit)

        Attachment #8798324 -
        Flags: review?(bugs)

        Attachment #8798324 -
        Flags: feedback?(wisniewskit)

    
    

    
  
Comment on attachment 8798324 [details] [diff] [review]
xhr.patch

oh, the change to the test wasn't needed.

        Attachment #8798324 -
        Flags: review?(bugs) → review+

    
    

    
  
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1932246b27f
XHR should set lengthComputable only if total value is set, r==smaug

    
    

    
      
  

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/b1932246b27f
Status: NEW → RESOLVED
Closed: 8 years ago

          status-firefox52:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

    
    

    
      
  

    
    

    
  
Comment on attachment 8798324 [details] [diff] [review]
xhr.patch

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

I realize this patch is already reviewed and checked-in, but I figured I'd nonetheless rubber-stamp it and clear the feedback request. Sorry for the delay!

        Attachment #8798324 -
        Flags: feedback?(wisniewskit) → feedback+
Depends on: 1308341
Depends on: 1308347
Depends on: 1308620
Component: DOM → DOM: Core & HTML

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: