1439455 - about:url-classifier should show the name of NS_ERROR types when an update fails

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P1)

Description

[François Marier [:francois]]

Currently when an update fails, we may get a message like "download error(<some large integer>)". See attached screenshot for an example.

It would be helpful to return something different from within the listmanager:
https://searchfox.org/mozilla-central/rev/0c0ddaa7e859a2b76a56a0e2e9c0de88af166812/toolkit/components/url-classifier/nsUrlClassifierListManager.js#633-654

so that we can quickly identify the cause of the failure.

Comment 1

[François Marier [:francois]]

Attachment: Screenshot

Comment 2

[François Marier [:francois]]

Attachment: Bug 1439455 - Round timestamps up to nearest minute in log messages.

Review commit: https://reviewboard.mozilla.org/r/222596/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222596/

Comment 3

[François Marier [:francois]]

Attachment: Bug 1439455 - Display error names instead of codes in about:url-classifier.

This also changes a few MOZ_LOG() messages to use the error name
instead of the raw numerical nsresult value.

Review commit: https://reviewboard.mozilla.org/r/222598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222598/

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8953319 [details]
Bug 1439455 - Round timestamps up to nearest minute in log messages.

https://reviewboard.mozilla.org/r/222596/#review229122

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8953320 [details]
Bug 1439455 - Display error names instead of codes in about:url-classifier.

https://reviewboard.mozilla.org/r/222598/#review229128

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:577
(Diff revision 1)
>    if (NS_SUCCEEDED(mUpdateStatus)) {
>      if (mProtocolParser->ResetRequested()) {
>        mClassifier->ResetTables(Classifier::Clear_All, mUpdateTables);
>      }
>    } else {
>      mUpdateStatus = NS_ERROR_UC_UPDATE_PROTOCOL_PARSER_ERROR;

We're in this branch if mUpdateStatus is set to an error.

Question: what failure modes are there that won't be "NS_ERROR_UC_UPDATE_PROTOCOL_PARSER_ERROR" and don't we want to propgate those instead?

Basically, it feels strange we are overwriting the original error here.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:582
(Diff revision 1)
>      mUpdateStatus = NS_ERROR_UC_UPDATE_PROTOCOL_PARSER_ERROR;
>    }
>  
>    mProtocolParser = nullptr;
>  
> -  return NS_OK;
> +  return mUpdateStatus;

Ouch! Nice catch.

Comment 6

[François Marier [:francois]]

Comment on attachment 8953320 [details]
Bug 1439455 - Display error names instead of codes in about:url-classifier.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222598/diff/1-2/

Comment 7

[François Marier [:francois]]

Comment on attachment 8953320 [details]
Bug 1439455 - Display error names instead of codes in about:url-classifier.

https://reviewboard.mozilla.org/r/222598/#review229128

> We're in this branch if mUpdateStatus is set to an error.
> 
> Question: what failure modes are there that won't be "NS_ERROR_UC_UPDATE_PROTOCOL_PARSER_ERROR" and don't we want to propgate those instead?
> 
> Basically, it feels strange we are overwriting the original error here.

Good point, I've removed that line in my last revision.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d6c681af2c2
Round timestamps up to nearest minute in log messages. r=gcp
https://hg.mozilla.org/integration/autoland/rev/67e9ab1a4745
Display error names instead of codes in about:url-classifier. r=gcp

Comment 9

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/2d6c681af2c2
https://hg.mozilla.org/mozilla-central/rev/67e9ab1a4745