Closed Bug 1439455 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Safe Browsing, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: francois, Assigned: francois)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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.
Attached image Screenshot
Assignee: nobody → francois
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8953319 [details]
Bug 1439455 - Round timestamps up to nearest minute in log messages.

https://reviewboard.mozilla.org/r/222596/#review229122
Attachment #8953319 - Flags: review?(gpascutto) → review+
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.
Attachment #8953320 - Flags: review?(gpascutto) → review+
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.
Pushed by fmarier@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/2d6c681af2c2
https://hg.mozilla.org/mozilla-central/rev/67e9ab1a4745
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.