Add missing prefs service failure checks

RESOLVED FIXED in Firefox 50

Status

()

Core
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

2 years ago
We have a few places in the code that don't check for failure when getting the prefs service.
(Assignee)

Comment 1

2 years ago
Created attachment 8760538 [details] [diff] [review]
(part 1) - Adding missing prefs service null checks in netwerk
Attachment #8760538 - Flags: review?(valentin.gosu)
(Assignee)

Updated

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created attachment 8760541 [details] [diff] [review]
(part 2) - Adding a missing prefs service null check in chrome/
Attachment #8760541 - Flags: review?(nfroyd)
(Assignee)

Comment 3

2 years ago
Created attachment 8760542 [details] [diff] [review]
(part 3) - Adding a missing prefs service null check in dom/flyweb/
Attachment #8760542 - Flags: review?(kvijayan)
(Assignee)

Comment 4

2 years ago
Created attachment 8760544 [details] [diff] [review]
(part 4) - Adding a missing prefs service null check in startupcache/
Attachment #8760544 - Flags: review?(nfroyd)
(Assignee)

Comment 5

2 years ago
Created attachment 8760545 [details] [diff] [review]
(part 5) - Adding a missing prefs service null check in url-classifier/
Attachment #8760545 - Flags: review?(gpascutto)
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.
(Assignee)

Comment 9

2 years ago
sicking: 12 day review ping
(Assignee)

Comment 10

2 years ago
(In reply to Nicholas Nethercote [:njn] from comment #9)
> sicking: 12 day review ping

Sicking: another ping for a trivial patch review.
(Assignee)

Comment 11

2 years ago
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.
(Assignee)

Comment 12

2 years ago
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
(Assignee)

Comment 14

2 years ago
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.
(Assignee)

Updated

2 years ago
Keywords: leave-open

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3e207bf67854
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.