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

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: bkelly)

Tracking

Trunk
mozilla28
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

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.
Posted 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: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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.