xmlhttprequest: Don't pass total value on timeout #24477

santoshs · 2019-10-17T13:40:01Z

Signed-off-by: Santosh Sivaraj santosh@fossix.org

Progress event should not get total/partial load values on timeout. Fix to pass 0 in the error path.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #24286

There are tests for these changes OR
These changes do not require tests because ___

highfive · 2019-10-17T13:40:10Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @SimonSapin (or someone else) soon.

highfive · 2019-10-17T13:40:11Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/xmlhttprequest.rs
@KiChjang: components/script/dom/xmlhttprequest.rs

highfive · 2019-10-17T13:40:13Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

jdm · 2019-10-18T05:07:02Z

@bors-servo try=wpt

bors-servo · 2019-10-18T05:07:07Z

⌛ Trying commit 569b083 with merge 529c384...


        Auto merge of #24477 - santoshs:gi24286-xhr, r=<try>

xmlhttprequest: Don't pass total value on timeout fixes: #24286 Signed-off-by: Santosh Sivaraj <santosh@fossix.org>  Progress event should not get total/partial load values on timeout. Fix to pass 0 in the error path. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #24286  - [X] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2019-10-18T05:44:13Z

💔 Test failed - linux-rel-wpt

CYBAI · 2019-10-18T06:22:23Z

Cool! More tests PASS now! but one (/xhr/send-response-event-order.htm) became FAIL from PASS 👀

{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "XMLHttpRequest: abort() while sending data", 
    "test": "/xhr/abort-during-upload.any.html", 
    "line": 220966, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "XMLHttpRequest: event - error (order of events)", 
    "test": "/xhr/event-error-order.sub.html", 
    "line": 256012, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "XMLHttpRequest: event - timeout (order of events)", 
    "test": "/xhr/event-timeout-order.any.worker.html", 
    "line": 321442, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "XMLHttpRequest: The abort() method: abort and loadend events", 
    "test": "/xhr/abort-event-order.htm", 
    "line": 332039, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "XMLHttpRequest: event - timeout (order of events)", 
    "test": "/xhr/event-timeout-order.any.html", 
    "line": 332411, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "FAIL", 
    "group": "default", 
    "message": "assert_equals: expected \"upload.progress(12,12,true)\" but got \"upload.progress(0,0,false)\"", 
    "stack": "global.assert_xhr_event_order_matches@http://web-platform.test:8000/xhr/resources/xmlhttprequest-event-order.js:74:7\n@http://web-platform.test:8000/xhr/send-response-event-order.htm:31:17\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1908:25\nTest.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1932:35\n", 
    "subtest": "XMLHttpRequest: The send() method: event order when synchronous flag is unset", 
    "test": "/xhr/send-response-event-order.htm", 
    "line": 332877, 
    "action": "test_result", 
    "expected": "PASS"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "XMLHttpRequest: abort() while sending data", 
    "test": "/xhr/abort-during-upload.any.worker.html", 
    "line": 348925, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "XMLHttpRequest: The send() method: timeout is not 0 ", 
    "test": "/xhr/send-timeout-events.htm", 
    "line": 363579, 
    "action": "test_result", 
    "expected": "FAIL"
}

jdm

With this change, we shouldn't have any new test failures.

components/script/dom/xmlhttprequest.rs

jdm · 2019-10-21T14:12:11Z

The expected test results will need to be updated to include the newly passing tests.

jdm · 2019-11-07T13:51:06Z

@santoshs Are you planning to finish this PR?

santoshs · 2019-11-08T04:17:31Z

@santoshs Are you planning to finish this PR?

@jdm Yes, I was on vacation, so the very late response. Will send a updated pull request today.


        xmlhttprequest: Don't pass total value on timeout

Progress event should not get total/partial load values on timeout. Fix to pass 0 in the error path. fixes: #24286 Signed-off-by: Santosh Sivaraj <santosh@fossix.org>

community-tc-integration · 2019-11-08T04:46:38Z

Submitting the task to Taskcluster failed. Details

Taskcluster-GitHub attempted to create a task for this event with the following scopes:

[
  "assume:repo:github.com/servo/servo:pull-request",
  "queue:route:statuses",
  "queue:scheduler-id:taskcluster-github"
]

The expansion of these scopes is not sufficient to create the task, leading to the following:

Client ID static/taskcluster/github does not have sufficient scopes and is missing the following scopes:

{
  "AnyOf": [
    {
      "AnyOf": [
        "queue:create-task:highest:aws-provisioner-v1/servo-docker-untrusted",
        "queue:create-task:very-high:aws-provisioner-v1/servo-docker-untrusted",
        "queue:create-task:high:aws-provisioner-v1/servo-docker-untrusted",
        "queue:create-task:medium:aws-provisioner-v1/servo-docker-untrusted",
        "queue:create-task:low:aws-provisioner-v1/servo-docker-untrusted",
        "queue:create-task:very-low:aws-provisioner-v1/servo-docker-untrusted",
        "queue:create-task:lowest:aws-provisioner-v1/servo-docker-untrusted"
      ]
    },
    {
      "AnyOf": [
        "queue:create-task:aws-provisioner-v1/servo-docker-untrusted",
        {
          "AllOf": [
            "queue:define-task:aws-provisioner-v1/servo-docker-untrusted",
            "queue:task-group-id:taskcluster-github/LHG4F0X7QhqI5vxn_bqQ_Q",
            "queue:schedule-task:taskcluster-github/LHG4F0X7QhqI5vxn_bqQ_Q/LHG4F0X7QhqI5vxn_bqQ_Q"
          ]
        }
      ]
    }
  ]
}

This request requires the client to satisfy the following scope expression:

{
  "AllOf": [
    "assume:repo:github.com/servo/servo:pull-request",
    "queue:route:tc-treeherder.v2._/servo-prs.08bf7ac536f7e989b563ca22f8112d0afb8eac75",
    "queue:route:tc-treeherder-staging.v2._/servo-prs.08bf7ac536f7e989b563ca22f8112d0afb8eac75",
    "queue:route:statuses",
    {
      "AnyOf": [
        {
          "AllOf": [
            "queue:scheduler-id:taskcluster-github",
            {
              "AnyOf": [
                "queue:create-task:highest:aws-provisioner-v1/servo-docker-untrusted",
                "queue:create-task:very-high:aws-provisioner-v1/servo-docker-untrusted",
                "queue:create-task:high:aws-provisioner-v1/servo-docker-untrusted",
                "queue:create-task:medium:aws-provisioner-v1/servo-docker-untrusted",
                "queue:create-task:low:aws-provisioner-v1/servo-docker-untrusted",
                "queue:create-task:very-low:aws-provisioner-v1/servo-docker-untrusted",
                "queue:create-task:lowest:aws-provisioner-v1/servo-docker-untrusted"
              ]
            }
          ]
        },
        {
          "AnyOf": [
            "queue:create-task:aws-provisioner-v1/servo-docker-untrusted",
            {
              "AllOf": [
                "queue:define-task:aws-provisioner-v1/servo-docker-untrusted",
                "queue:task-group-id:taskcluster-github/LHG4F0X7QhqI5vxn_bqQ_Q",
                "queue:schedule-task:taskcluster-github/LHG4F0X7QhqI5vxn_bqQ_Q/LHG4F0X7QhqI5vxn_bqQ_Q"
              ]
            }
          ]
        }
      ]
    }
  ]
}

method: createTask
errorCode: InsufficientScopes
statusCode: 403
time: 2019-11-08T04:46:38.573Z

jdm · 2019-11-08T12:59:22Z

components/script/dom/xmlhttprequest.rs

+                    self.dispatch_upload_progress_event(atom!("progress"), Err(()));
                    return_if_fetch_was_terminated!();
-                    self.dispatch_upload_progress_event(atom!("load"), None);
+                    self.dispatch_upload_progress_event(atom!("load"), Err(()));


These two lines should use Ok(Some(0)), since this is not an error code path.

@jdm thanks for the review. The servo build most of time gets killed because of lack of memory, I have a 8 GB system. I have read somewhere that using llvm linker would use less memory, any idea how to set that up? I have a fedora 30 machine.

Based on rust-lang/rust#39915 (comment) and rust-lang/rust#39915 (comment), I think you would need to follow these instructions and modify the rustflags entry to include -C linker=rust-lld.

jdm · 2020-01-21T14:22:34Z

Closing due to inactivity.

highfive assigned SimonSapin Oct 17, 2019

highfive added the S-awaiting-review label Oct 17, 2019

santoshs force-pushed the santoshs:gi24286-xhr branch from 339b6f8 to 569b083 Oct 18, 2019

highfive added the S-tests-failed label Oct 18, 2019

jdm requested changes Oct 21, 2019

View changes

components/script/dom/xmlhttprequest.rs Outdated Show resolved Hide resolved

jdm assigned jdm and unassigned SimonSapin Oct 21, 2019

jdm added S-needs-code-changes and removed S-awaiting-review labels Oct 21, 2019

xmlhttprequest: Don't pass total value on timeout

Loading status checks…

08bf7ac

Progress event should not get total/partial load values on timeout. Fix to pass 0 in the error path. fixes: #24286 Signed-off-by: Santosh Sivaraj <santosh@fossix.org>

santoshs force-pushed the santoshs:gi24286-xhr branch from 569b083 to 08bf7ac Nov 8, 2019

highfive added S-awaiting-review and removed S-tests-failed S-needs-code-changes labels Nov 8, 2019

jdm requested changes Nov 8, 2019

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 8, 2019

jdm mentioned this pull request Nov 29, 2019

Timed-out, aborted, and errored XHR incorrectly reporting non-zero bytes loaded #24926

Open

jdm added this to In progress in web-platform-test failures Nov 29, 2019

jdm closed this Jan 21, 2020

web-platform-test failures automation moved this from In progress to Done Jan 21, 2020

jdm mentioned this pull request Jan 21, 2020

XHR upload timeout event should not include request progress/total values #24286

Closed

servo / servo

xmlhttprequest: Don't pass total value on timeout #24477

xmlhttprequest: Don't pass total value on timeout #24477

santoshs commented Oct 17, 2019

highfive commented Oct 17, 2019

highfive commented Oct 17, 2019

highfive commented Oct 17, 2019

jdm commented Oct 18, 2019

bors-servo commented Oct 18, 2019

bors-servo commented Oct 18, 2019

CYBAI commented Oct 18, 2019

jdm left a comment

jdm commented Oct 21, 2019

jdm commented Nov 7, 2019

santoshs commented Nov 8, 2019

This comment has been minimized.

community-tc-integration bot commented on `08bf7ac` Nov 8, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Jan 21, 2020

servo / servo

Join GitHub today

xmlhttprequest: Don't pass total value on timeout #24477

xmlhttprequest: Don't pass total value on timeout #24477

Conversation

santoshs commented Oct 17, 2019

highfive commented Oct 17, 2019

highfive commented Oct 17, 2019

highfive commented Oct 17, 2019

jdm commented Oct 18, 2019

bors-servo commented Oct 18, 2019

bors-servo commented Oct 18, 2019

CYBAI commented Oct 18, 2019

jdm left a comment

jdm commented Oct 21, 2019

jdm commented Nov 7, 2019

santoshs commented Nov 8, 2019

This comment has been minimized.

community-tc-integration bot commented on 08bf7ac Nov 8, 2019

This comment has been minimized.

jdm Nov 8, 2019

This comment has been minimized.

santoshs Nov 9, 2019

This comment has been minimized.

jdm Nov 9, 2019

jdm commented Jan 21, 2020

community-tc-integration bot commented on `08bf7ac` Nov 8, 2019