Closed Bug 943414 Opened 10 years ago Closed 10 years ago

Warn appropriately if consumers pass a non-null aPrompt to nsICookieService::SetCookieString

Categories

(Core :: Networking: Cookies, defect)

Product:
Core
Component:
Networking: Cookies
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla28
Tracking Flags:
Tracking Status
firefox28 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: bkelly)

References

Details

(Whiteboard: [c= p=1 s= u=][qa-])

Attachments

(1 file, 1 obsolete file)

cookie-prompt-warn.patch
3.05 KB, patch
bkelly
: review+
Details | Diff | Splinter Review 
See bug 932418 for an example of how this can screw us.  We should abort in debug builds and warn something to the error console in opt builds if you pass a non-null argument as aPrompt.
I wonder if this could be more generic, e.g. tag any idl arguments as deprecated.
Tagging the IDL as deprecated wouldn't affect direct C++ callers in any way, right?
Yes it won't, and it's beyond the scope of this bug per se.
Attached patch cookie-prompt-warn.patch (obsolete) — — Splinter Review 
Here's a candidate patch.  Try:

  https://tbpl.mozilla.org/?tree=Try&rev=1387bb9e08e3
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
We need the fix in bug 932418 to avoid blowing up tests on debug builds with the assert added here.
Depends on: 932418
Whiteboard: [c= p=1 s= u=]
Comment on attachment 8339296 [details] [diff] [review]
cookie-prompt-warn.patch

Try looks green, asking for review.
Attachment #8339296 - Flags: review?(ehsan)
Comment on attachment 8339296 [details] [diff] [review]
cookie-prompt-warn.patch

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

r=me with the below.

::: netwerk/cookie/nsCookieService.cpp
@@ +1583,5 @@
> +  MOZ_ASSERT(!aPrompt);
> +  if (aPrompt) {
> +    nsCOMPtr<nsIConsoleService> aConsoleService =
> +        do_GetService("@mozilla.org/consoleservice;1");
> +    aConsoleService->LogStringMessage(

Please null-check the service before using it.
Attachment #8339296 - Flags: review?(ehsan) → review+
Add null check for console service.  Carry forward r+ from Ehsan.  Update commit message to indicate r=ehsan.

Sanity check try:

  https://tbpl.mozilla.org/?tree=Try&rev=6b4fb42dc9f0
Attachment #8339296 - Attachment is obsolete: true
Attachment #8341094 - Flags: review+
Adding checkin-needed.  Please land bug 932418 first.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ea87ed77256e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
status-firefox28: --- → fixed
Whiteboard: [c= p=1 s= u=] → [c= p=1 s= u=][qa-]
You need to log in before you can comment on or make changes to this bug.