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

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[[:philipp]]

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?

Comment 1

[Dragana Damjanovic [:dragana]]

Is it something that will be fixed with your patches?

Comment 2

[Amy Chung [:Amy]]

Ok, I will take a look at this bug.

Comment 3

[Amy Chung [:Amy]]

Attachment: implementation -- Created mIPCOpen and added if condition before sending ipc msg to parent.

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!

Comment 4

[Josh Matthews [:jdm]]

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.

Comment 5

[Amy Chung [:Amy]]

Attachment: implementation -- Created mIPCOpen and added if condition before sending ipc msg to parent.

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!

Comment 6

[Josh Matthews [:jdm]]

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.

Comment 7

[Amy Chung [:Amy]]

Attachment: implementation -- Created mIPCOpen and added if condition before sending ipc msg to parent.

Comment 8

[Amy Chung [:Amy]]

[Try server result]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=72905890545254e0e9251521cf3ff141005f8b61&selectedJob=124048526

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 10

[Amy Chung [:Amy]]

Attachment: implementation -- Created mIPCOpen and added if condition before sending ipc msg to parent.

Comment 11

[Amy Chung [:Amy]]

[Try Server Result]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43ae2b2dcfc378ef3b37d27696bbad8d5d375ef5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d214c46769a
Add mIPCOpen flag and check it before sending IPC messages to the parent. r=jdm

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2d214c46769a

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 17

[Amy Chung [:Amy]]

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

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8899783 [details] [diff] [review]
implementation -- Created mIPCOpen and added if condition before sending ipc msg to parent.

Fix a crash. Beta56+.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

This needs a rebased patch for Beta uplift.

Comment 20

[Amy Chung [:Amy]]

Attachment: 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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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 :)

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/32b050fa7a9b