Update error messages are often incorrect

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

This should also cover the test for bug 312661
(In reply to comment #0)
> test for bug 312661

See my Bv0 patch there for cases to check...
Depends on: 312661
Posted patch patch in progress (obsolete) — Splinter Review
I may not get to this soon so if anyone wants to run with it please do so.
Summary: Create a general test for XMLHttpRequest request.status and update.statusText → Update error messages are often incorrect
Posted patch patch (obsolete) — Splinter Review
Often times the download of the xml completes successfully when there are errors.
Assignee: nobody → robert.bugzilla
Attachment #334429 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #334643 - Flags: review?(dtownsend)
Comment on attachment 334643 [details] [diff] [review]
patch

Maybe rename run_test_helper_pt1 to run_test_helper and same for check_test_helper_pt1 since they are used for all parts of the test, but otherwise looks fine.
Attachment #334643 - Flags: review?(dtownsend) → review+
This includes a bare bones implementation of XMLHttpRequest so the test can return status values we can't simulate with XMLHttpRequest and the test httpd. Where ever possible it uses the actual XMLHttpRequest implementation.
Attachment #334971 - Flags: review?(dtownsend)
Carrying forward review
Attachment #334643 - Attachment is obsolete: true
Attachment #334973 - Flags: review+
Attachment #334971 - Attachment is obsolete: true
Attachment #334973 - Attachment is obsolete: true
Attachment #335286 - Flags: review+
Attachment #334971 - Flags: review?(dtownsend)
Leaving open to get the additional tests in
Comment on attachment 335286 [details] [diff] [review]
original patch with nits fixed (checked in)

>diff --git a/toolkit/mozapps/update/src/nsUpdateService.js.in b/toolkit/mozapps/update/src/nsUpdateService.js.in
>+    }
>+      LOG("Checker", "status: " + status);
>+    if (status == 0)

Nit: LOG line is misaligned.
Dave, this is essentially the same patch that also fixes bug 312661 since the tests are from this bug.
Attachment #335422 - Flags: review?(dtownsend)
Marking this fixed though and will land the additional tests once they are approved
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: wanted1.9.0.x?
Resolution: --- → FIXED
Comment on attachment 335287 [details] [diff] [review]
additional tests

These tests don't actually run here.

>+function overrideXHR(callback) {
>+  gXHR = new xhr();
>+  var registrar = Components.manager.QueryInterface(AUS_Ci.nsIComponentRegistrar);
>+  registrar.registerFactory(xhr.prototype.classID, xhr.prototype.classDescription,
>+                            xhr.prototype.contractID, gXHR);
>+  gXHR.callback = callback;
>+}

Here you set callback on a new xhr object. But the createInstance method in xhr returns another new instance, which won't have callback defined. You could maybe set xhr.prototype.callback, or make createInstance always return gXHR. Both will have issues if any code under test tries to use two xhr's simultaneously though I think.

>+
>+/**
>+ * Bare bones XMLHttpRequest implementation for testing onprogress, onerror,
>+ * and onload nsIDomEventListener handleEvent.
>+ */
>+function xhr() {
>+}
>+xhr.prototype = {
>+  callback: null,
>+  overrideMimeType: function(mimetype) { },
>+  setRequestHeader: function(header, value) { },
>+  status: null,
>+  channel: { set notificationCallbacks(val) { } },
>+  _utl: null,

I think you meant _url

>+  _method: null,
>+  open: function (method, url) { this._method = method; this._url = url; },
>+  send: function(body) {
>+    gXHR = this;
>+    do_timeout(0, "callback()"); // Use a timeout so the XHR completes
>+  },

This is kinda ugly, if two xhr's are created and run in quick succession then the second will be the global gXHR when the callback is executed for the first. Also callback is not a defined function anywhere, possibly you wanted "gXHR.callback()".

>+function run_test() {
>+  do_test_pending();
>+  startAUS();
>+  overrideXHR(callHanleEvent);

callHandleEvent ;)

>+// Callback function used by the custom XMLHttpRequest implemetation to
>+// call the nsIDOMEventListener's handleEvent method for onload.
>+function callHanleEvent() {
>+  gXHR.status = gExpectedStatus;

gExpectedStatus is undeclared
Attachment #335287 - Flags: review?(dtownsend) → review-
> This is kinda ugly, if two xhr's are created and run in quick succession then
> the second will be the global gXHR when the callback is executed for the first.
I don't think these or any of the update tests will ever need to be concerned with using two xhr's using a custom xhr implementation... if they do in the future then it can be rewritten to support it though I bet it could use the real implementation.

> These tests don't actually run here.
Could you attach the log output from the original patch? It was succeeding on my system (though it finishes much faster likely due to the custom xhr) as can be seen by the onError callbacks to the listener as follows. Problems with the original test aside I'm curious why it would fail on your system and not mine.
Testing: failed (unknown reason) - http://localhost:4444/data/aus-0051_general-1.xml
onError: request.status = 2152398849, update.statusText = Failed (Unknown Reason)
Attachment #335287 - Attachment is obsolete: true
Attachment #335576 - Flags: review?(dtownsend)
I was just getting:

*** test pending
*** test pending
*** test finished
*** running event loop
Testing: failed (unknown reason) - http://localhost:4444/data/aus-0051_general-1.xml

then it would sit there waiting.

I'll retry things tomorrow on different platforms to see if it a Mac oddity.
Comment on attachment 335576 [details] [diff] [review]
additional test patch rev2 (checked in)

This version works much better.
Attachment #335576 - Flags: review?(dtownsend) → review+
Attachment #335422 - Flags: review?(dtownsend) → review+
Comment on attachment 335422 [details] [diff] [review]
patch for 1.9.0.x

Drivers, I'd like to get this for 1.9.0.3. It is well tested and safe.
Attachment #335422 - Flags: approval1.9.0.3?
Comment on attachment 335576 [details] [diff] [review]
additional test patch rev2 (checked in)

Checked in the additional tests
http://hg.mozilla.org/mozilla-central/index.cgi/rev/c4fa1f905ea7e79ba7c7d89a844ae67cefc8b338
Attachment #335576 - Attachment description: additional test patch rev2 → additional test patch rev2 (checked in)

Comment 24

11 years ago
As described in bug 440750, this bug changes an error in checkCert from one wrong error message to another wrong error message.

I would request that this is not landed on 1.9.0.x as is, as it would make documenting a frequent error more complicated.

Possible solutions could be fixing bug 440750, fixing bug 401292 or special-casing checkCert errors to the 404 message.
Jesper, I'll remove the request for now and make this bug depend on bug 401292 for tracking purposes. If bug 401292 isn't fixed I might make this fallback to the 404 for 1.9.0.x since bug 401292 will most likely require string changes.

btw: bug 401292 is simple to fix and I'll most likely have a patch this coming week.
Depends on: 401292
You need to log in before you can comment on or make changes to this bug.