Closed Bug 1376009 Opened 3 years ago Closed 3 years ago

Crash in mozalloc_abort | NS_DebugBreak | mozilla::ipc::LogicError | mozilla::net::PCookieServiceChild::SendGetCookieString

Categories

(Core :: Networking: Cookies, defect, critical)

55 Branch
x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- wontfix
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: philipp, Assigned: amchung)

References

Details

(Keywords: crash, Whiteboard: [necko-active])

Crash Data

Attachments

(1 file, 4 obsolete files)

This bug was filed from the Socorro interface and is 
report bp-a6a8a718-9564-4389-8461-c51b40170623.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	mozglue.dll 	mozalloc_abort(char const* const) 	memory/mozalloc/mozalloc_abort.cpp:33
1 	xul.dll 	NS_DebugBreak 	xpcom/base/nsDebugImpl.cpp:438
2 	xul.dll 	mozilla::ipc::LogicError(char const*) 	ipc/glue/ProtocolUtils.cpp:312
3 	xul.dll 	mozilla::net::PCookieServiceChild::SendGetCookieString(mozilla::ipc::URIParams const&, bool const&, mozilla::OriginAttributes const&, nsCString*) 	obj-firefox/ipc/ipdl/PCookieServiceChild.cpp:66
4 	xul.dll 	mozilla::net::CookieServiceChild::GetCookieStringInternal(nsIURI*, nsIChannel*, char**) 	netwerk/cookie/CookieServiceChild.cpp:143
5 	xul.dll 	mozilla::net::CookieServiceChild::GetCookieString(nsIURI*, nsIChannel*, char**) 	netwerk/cookie/CookieServiceChild.cpp:213
6 	xul.dll 	nsHTMLDocument::GetCookie(nsAString&, mozilla::ErrorResult&) 	dom/html/nsHTMLDocument.cpp:1352
7 	xul.dll 	mozilla::dom::HTMLDocumentBinding::get_cookie 	obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:103
8 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:470
9 	xul.dll 	js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp:534
10 	xul.dll 	CallGetter 	js/src/vm/NativeObject.cpp:2082
11 	xul.dll 	js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) 	js/src/vm/NativeObject.h:1538
12 	xul.dll 	mozilla::dom::GetPropertyOnPrototype(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, bool*, JS::MutableHandle<JS::Value>) 	dom/bindings/BindingUtils.cpp:2033
13 	xul.dll 	mozilla::dom::HTMLDocumentBinding::DOMProxyHandler::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) 	obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:2155
14 	xul.dll 	js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) 	js/src/proxy/Proxy.cpp:340
15 	xul.dll 	GetPropertyOperation 	js/src/vm/Interpreter.cpp:193
...

the signature was appearing since nightly 55.0a1 build 20170602030204, commonly during shutdown. maybe similar to bug 1374939?
Is it something that will be fixed with your patches?
Flags: needinfo?(amchung)
Ok, I will take a look at this bug.
Flags: needinfo?(amchung)
Assignee: nobody → amchung
Whiteboard: [necko-active]
Hi Josh,
I have modified this patch as below:
[CookieServiceChild]
1. Created mIPCOpen.
2. Override ActorDestroy() on CookieServiceChild.
3. Set mIPCOpen to true on constructor of CookieServiceChild.
4. Set mIPCOpen to false on ActorDestroy.
5. Created if conditions for confirming mIPCOpen is true or not before CookieServiceChild sent ipc msg to CookieServiceParent.

Would you help me to review this patch?
Thanks!
Attachment #8894765 - Flags: review?(josh)
Comment on attachment 8894765 [details] [diff] [review]
implementation -- Created mIPCOpen and added if condition before sending ipc msg to parent.

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

::: netwerk/cookie/CookieServiceChild.cpp
@@ +320,5 @@
>                                             bool                   aIsForeign,
>                                             const OriginAttributes &aAttrs,
>                                             nsAutoCString          &aCookieString)
>  {
> +  if (!mIPCOpen) {

Let's remove this and check before calling in GetCookieStringInternal. Then we can return NS_ERROR_NOT_AVAILABLE instead.
Attachment #8894765 - Flags: review?(josh) → review+
Hi Josh,
Previous patch occurred memory leak on try server, I fixed this error as below:

[CookieServiceChild]
1. Moved "gCookieService = null" on destructor.


Would you help me to review this patch?
Thanks!
Attachment #8894765 - Attachment is obsolete: true
Attachment #8897926 - Flags: review?(josh)
Comment on attachment 8897926 [details] [diff] [review]
implementation -- Created mIPCOpen and added if condition before sending ipc msg to parent.

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

::: netwerk/cookie/CookieServiceChild.cpp
@@ +320,5 @@
>                                             const OriginAttributes &aAttrs,
>                                             nsAutoCString          &aCookieString)
>  {
> +  if (!mIPCOpen) {
> +    return;

Please address my comment from the last review.
Attachment #8897926 - Flags: review?(josh) → review+
Attachment #8897926 - Attachment is obsolete: true
Attachment #8898595 - Attachment description: mplementation -- Created mIPCOpen and added if condition before sending ipc msg to parent. → implementation -- Created mIPCOpen and added if condition before sending ipc msg to parent.
Needs rebasing.
Flags: needinfo?(amchung)
Keywords: checkin-needed
Attachment #8898595 - Attachment is obsolete: true
In the future, *please* make sure the patches you attach have all the proper commit information in them before requesting checkin. It really slows things down when I have to add it manually later.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d214c46769a
Add mIPCOpen flag and check it before sending IPC messages to the parent. r=jdm
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2d214c46769a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Duplicate of this bug: 1374939
Please request Beta approval on this when you get a chance.
Flags: needinfo?(amchung)
Comment on attachment 8899783 [details] [diff] [review]
implementation -- Created mIPCOpen and added if condition before sending ipc msg to parent.

Approval Request Comment
[Feature/Bug causing the regression]:unknown
[User impact if declined]:unknown
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:no
[Needs manual test from QE? If yes, steps to reproduce]:unknown
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:no
[Why is the change risky/not risky?]:Only added a mIPCOpen flag to verify if CookiSerivceChild can send ipc message to Parent.
[String changes made/needed]:none
Flags: needinfo?(amchung)
Attachment #8899783 - Flags: approval-mozilla-beta?
Comment on attachment 8899783 [details] [diff] [review]
implementation -- Created mIPCOpen and added if condition before sending ipc msg to parent.

Fix a crash. Beta56+.
Attachment #8899783 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This needs a rebased patch for Beta uplift.
Flags: needinfo?(amchung)
Approval Request Comment
[Feature/Bug causing the regression]:unknown
[User impact if declined]:unknown
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:no
[Needs manual test from QE? If yes, steps to reproduce]:unknown
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:no
[Why is the change risky/not risky?]:Only added a mIPCOpen flag to verify if CookiSerivceChild can send ipc message to Parent.
[String changes made/needed]:none
Attachment #8899783 - Attachment is obsolete: true
Flags: needinfo?(amchung)
Attachment #8902655 - Flags: review+
Attachment #8902655 - Flags: approval-mozilla-beta?
Comment on attachment 8902655 [details] [diff] [review]
implementation -- Created mIPCOpen and added if condition before sending ipc msg to parent.

Doesn't need re-approval. Thanks for the rebased patch :)
Attachment #8902655 - Flags: approval-mozilla-beta?
You need to log in before you can comment on or make changes to this bug.