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

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 1

[Quentin Headen [:qheaden]]

This bug seems simple enough. I'll work on a fix.

Comment 2

[Quentin Headen [:qheaden]]

Are there any tests for this error that I will need to update for the new error message?

Comment 3

[Quentin Headen [:qheaden]]

Attachment: Patch 1

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.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 5

[Quentin Headen [:qheaden]]

Attachment: Patch 2

This updated places the URL in a better location within the error message.

Comment 6

[Quentin Headen [:qheaden]]

Attachment: Patch 2 (Formatting Fixed)

Oops. I made a mistake on the formatting of one line. This patch fixes it.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 8

[Quentin Headen [:qheaden]]

Attachment: Patch 3

Okay. I fixed consistency. Not sure why I made that last mistake. I've got to stop coding so late at night. :)

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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...

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

And landed on the fx-team branch. Thanks, Quentin :)

https://hg.mozilla.org/integration/fx-team/rev/1c0e964bd620

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1c0e964bd620