Closed Bug 802454 Opened 7 years ago Closed 7 years ago

Log messages for 404 errors when checking for add-on updates should include the URL

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: Unfocused, Assigned: qheaden)

Details

Attachments

(1 file, 3 obsolete files)

At the moment, when an update update check fails due to a 404, we log the following warning:

WARN addons.updates: Request failed: 404: Not Found" {file: "resource://gre/modules/AddonUpdateChecker.jsm" line: 470}



That would be much more useful if it included the URL.
This bug seems simple enough. I'll work on a fix.
Assignee: nobody → qheaden
Are there any tests for this error that I will need to update for the new error message?
Attached patch Patch 1 (obsolete) — Splinter Review
Here's the first patch to the issue. It basically stores the URL given to the constructor inside of an instance variable,and uses that variable in the error message.

I'm not sure if there are any tests that need to be changed or created.
Attachment #727759 - Flags: feedback?(bmcbride)
Comment on attachment 727759 [details] [diff] [review]
Patch 1

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

No tests to update/create - we don't test log messages.

::: toolkit/mozapps/extensions/AddonUpdateChecker.jsm
@@ +463,5 @@
>  
>      let channel = request.channel;
>      if (channel instanceof Ci.nsIHttpChannel && !channel.requestSucceeded) {
> +      WARN("Request failed: " + channel.responseStatus + ": " + channel.responseStatusText +
> +           "- URL: " + this.url);

Lets put the URL before the status, as the URL is typically what I first look for. So it ends up something like:
  Request failed: https://whatever.tld/something - 500: Server error
Attachment #727759 - Flags: feedback?(bmcbride) → feedback+
Status: NEW → ASSIGNED
Attached patch Patch 2 (obsolete) — Splinter Review
This updated places the URL in a better location within the error message.
Attachment #727759 - Attachment is obsolete: true
Attachment #731410 - Flags: feedback?(bmcbride)
Attached patch Patch 2 (Formatting Fixed) (obsolete) — Splinter Review
Oops. I made a mistake on the formatting of one line. This patch fixes it.
Attachment #731410 - Attachment is obsolete: true
Attachment #731410 - Flags: feedback?(bmcbride)
Attachment #731411 - Flags: feedback?(bmcbride)
Comment on attachment 731411 [details] [diff] [review]
Patch 2 (Formatting Fixed)

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

::: toolkit/mozapps/extensions/AddonUpdateChecker.jsm
@@ +510,5 @@
>      else if (this.request.channel instanceof Ci.nsIHttpChannel) {
>        try {
>          if (this.request.channel.requestSucceeded) {
>            WARN("Request failed: " + this.request.channel.responseStatus + ": " +
> +               this.url + " - " + this.request.channel.responseStatusText);

This is inconsistent with the one above.
Attachment #731411 - Flags: feedback?(bmcbride) → feedback-
Attached patch Patch 3Splinter Review
Okay. I fixed consistency. Not sure why I made that last mistake. I've got to stop coding so late at night. :)
Attachment #731411 - Attachment is obsolete: true
Attachment #731745 - Flags: feedback?(bmcbride)
Comment on attachment 731745 [details] [diff] [review]
Patch 3

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

(In reply to Quentin Headen from comment #8)
> I've got to stop coding so late at night. :)

Boy do I know that feeling...
Attachment #731745 - Flags: feedback?(bmcbride) → review+
And landed on the fx-team branch. Thanks, Quentin :)

https://hg.mozilla.org/integration/fx-team/rev/1c0e964bd620
https://hg.mozilla.org/mozilla-central/rev/1c0e964bd620
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.