Closed Bug 1306235 Opened 3 years ago Closed 3 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)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
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)

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.patchSplinter 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: 3 years ago
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.