Closed
Bug 943414
Opened 11 years ago
Closed 11 years ago
Warn appropriately if consumers pass a non-null aPrompt to nsICookieService::SetCookieString
Categories
(Core :: Networking: Cookies, defect)
Tracking
()
RESOLVED
FIXED
mozilla28
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)
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.
Comment 1•11 years ago
|
||
I wonder if this could be more generic, e.g. tag any idl arguments as deprecated.
![]() |
||
Comment 2•11 years ago
|
||
Tagging the IDL as deprecated wouldn't affect direct C++ callers in any way, right?
Reporter | ||
Comment 3•11 years ago
|
||
Yes it won't, and it's beyond the scope of this bug per se.
Assignee | ||
Comment 4•11 years ago
|
||
Here's a candidate patch. Try:
https://tbpl.mozilla.org/?tree=Try&rev=1387bb9e08e3
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•11 years ago
|
||
We need the fix in bug 932418 to avoid blowing up tests on debug builds with the assert added here.
Depends on: 932418
Assignee | ||
Updated•11 years ago
|
Whiteboard: [c= p=1 s= u=]
Assignee | ||
Comment 6•11 years ago
|
||
Comment on attachment 8339296 [details] [diff] [review]
cookie-prompt-warn.patch
Try looks green, asking for review.
Attachment #8339296 -
Flags: review?(ehsan)
Reporter | ||
Comment 7•11 years ago
|
||
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+
Assignee | ||
Comment 8•11 years ago
|
||
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+
Assignee | ||
Comment 9•11 years ago
|
||
Adding checkin-needed. Please land bug 932418 first.
Keywords: checkin-needed
Reporter | ||
Comment 10•11 years ago
|
||
Keywords: checkin-needed
Comment 11•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 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.
Description
•