Closed Bug 1278439 Opened 3 years ago Closed 3 years ago

Add missing prefs service failure checks

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(5 files)

We have a few places in the code that don't check for failure when getting the prefs service.
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Comment on attachment 8760538 [details] [diff] [review]
(part 1) - Adding missing prefs service null checks in netwerk

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

::: netwerk/cache/nsCacheService.cpp
@@ +415,5 @@
> +        // njn: not sure if this is the best way to handle this one
> +        if (!branch) {
> +            return NS_ERROR_FAILED;
> +        }
> +        (void)ReadPrefs(branch);

Maybe it we should add the null check to ReadPrefs instead?

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +193,5 @@
>      mDelaysDisabled = false;
>  
>      nsCOMPtr<nsIPrefBranch> prefService =
>        do_GetService(NS_PREFSERVICE_CONTRACTID);
> +    if (prefService) {

wouldn't it be better to use if (!prefService) { return; } ?
Attachment #8760538 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8760542 [details] [diff] [review]
(part 3) - Adding a missing prefs service null check in dom/flyweb/

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

It seems we don't use the prefService var at all?  Seems ok to me.  Forwarding to jonas to confirm.
Attachment #8760542 - Flags: review?(kvijayan) → review?(jonas)
Attachment #8760545 - Flags: review?(gpascutto) → review+
Attachment #8760541 - Flags: review?(nfroyd) → review+
Attachment #8760544 - Flags: review?(nfroyd) → review+
I do wonder how many of these checks and the checks we already have result in never-taken branches.
sicking: 12 day review ping
(In reply to Nicholas Nethercote [:njn] from comment #9)
> sicking: 12 day review ping

Sicking: another ping for a trivial patch review.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e5e8ecc28e1d7f7645652788709e07356a37ac
Bug 1278439 - Adding missing prefs service null checks in netwerk/. r=valentin.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d31ab2eab0a85b09b80f6726d847619ec2b9d846
Bug 1278439 - Adding a missing prefs service null check in chrome/. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/00e10deea23befbf32097b0df836e11b42170f19
Bug 1278439 - Adding a missing prefs service null check in startupcache/. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0948b9cc4d82c3bf94abff04fecfaf06217fbeda
Bug 1278439 - Adding a missing prefs service null check in url-classifier/. r=gcp.
leave-open because I'm still waiting on sicking's review. djvj, if you want to be bold and r+ the trivial patch, I'll happily take it :)
Keywords: leave-open
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e207bf6785484173d3131504417cb78c706f738
Bug 1278439 (part 3) - Adding a missing prefs service null check in dom/flyweb/. r=djvj.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/3e207bf67854
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.