Closed
Bug 1376009
Opened 7 years ago
Closed 7 years ago
Crash in mozalloc_abort | NS_DebugBreak | mozilla::ipc::LogicError | mozilla::net::PCookieServiceChild::SendGetCookieString
Categories
(Core :: Networking: Cookies, defect)
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)
4.07 KB,
patch
|
amchung
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
Is it something that will be fixed with your patches?
Flags: needinfo?(amchung)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → amchung
Updated•7 years ago
|
Whiteboard: [necko-active]
Assignee | ||
Comment 3•7 years ago
|
||
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 4•7 years ago
|
||
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+
Assignee | ||
Comment 5•7 years ago
|
||
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 6•7 years ago
|
||
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+
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8898595 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8897926 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
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.
Assignee | ||
Comment 8•7 years ago
|
||
[Try server result] https://treeherder.mozilla.org/#/jobs?repo=try&revision=72905890545254e0e9251521cf3ff141005f8b61&selectedJob=124048526
Keywords: checkin-needed
Assignee | ||
Comment 10•7 years ago
|
||
Flags: needinfo?(amchung)
Attachment #8899783 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8898595 -
Attachment is obsolete: true
Assignee | ||
Comment 11•7 years ago
|
||
[Try Server Result] https://treeherder.mozilla.org/#/jobs?repo=try&revision=43ae2b2dcfc378ef3b37d27696bbad8d5d375ef5
Keywords: checkin-needed
Comment 12•7 years ago
|
||
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•7 years ago
|
||
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
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2d214c46769a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Comment 16•7 years ago
|
||
Please request Beta approval on this when you get a chance.
Flags: needinfo?(amchung)
Assignee | ||
Comment 17•7 years ago
|
||
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 18•7 years ago
|
||
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+
Assignee | ||
Comment 20•7 years ago
|
||
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 21•7 years ago
|
||
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?
Comment 22•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/32b050fa7a9b
You need to log in
before you can comment on or make changes to this bug.
Description
•