Closed
Bug 1100205
Opened 10 years ago
Closed 10 years ago
Zero ScreenDetails outparam in RecvScreenForBrowser() so Valgrind doesn't complain
Categories
(Core :: DOM: Content Processes, defect)
Core
DOM: Content Processes
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(1 file)
1.17 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
There's one undefined value error left on Try when I run the valgrind test with e10s enabled. It's another case where sendmsg() is passed undefined bytes. Here's the complaint after I added a VALGRIND_CHECK_MEM_IS_DEFINED request in Pickle::WriteBytes, which results in easier-to-understand errors: > Uninitialised byte(s) found during client check request > at 0x85C784D: Pickle::WriteBytes(void const*, int, unsigned int) (pickle.cc:564) > by 0x877D752: WriteUInt32 (pickle.h:133) > by 0x877D752: Write (ipc_message_utils.h:297) > by 0x877D752: WriteParam<unsigned int> (ipc_message_utils.h:115) > by 0x877D752: Write<unsigned int> (PScreenManagerParent.h:239) > by 0x877D752: mozilla::dom::PScreenManagerParent::Write(mozilla::dom::ScreenDetails const&, IPC::Message*) (PScreenManagerParent.cpp:505) > by 0x877E123: mozilla::dom::PScreenManagerParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (PScreenManagerParent.cpp:365) > by 0x86A085F: mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (PContentParent.cpp:4640) > Address 0xffeffe7c4 is on thread 1's stack > in frame #1, created by mozilla::dom::PScreenManagerParent::Write(mozilla::dom::ScreenDetails const&, IPC::Message*) (pickle.h:504) > Uninitialised value was created by a stack allocation > at 0x877DA94: mozilla::dom::PScreenManagerParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (PScreenManagerParent.cpp:177) Around PScreenManagerParent.cpp:365 we have this generated code: > ScreenDetails screen; > bool success; > int32_t __id = mId; > if ((!(RecvScreenForBrowser(aBrowser, (&(screen)), (&(success)))))) > { > mozilla::ipc::ProtocolErrorBreakpoint("Handler for ScreenForBrowser returned error code"); > return MsgProcessingError; > } > > __reply = new PScreenManager::Reply_ScreenForBrowser(__id); > > Write(screen, __reply); // line 365 Around PScreenManagerParent.cpp:505 we have this generated code: > auto PScreenManagerParent::Write( > const ScreenDetails& __v, > Message* __msg) -> void > { > Write((__v).id(), __msg); // line 505 > Write((__v).rect(), __msg); > Write((__v).rectDisplayPix(), __msg); > Write((__v).availRect(), __msg); > Write((__v).availRectDisplayPix(), __msg); > Write((__v).pixelDepth(), __msg); // line 512 > Write((__v).colorDepth(), __msg); // line 513 > Write((__v).contentsScaleFactor(), __msg); // line 514 > } Valgrind complains about lines 505, 512, 513 and 514, which correspond to the POD members of ScreenDetails, which are the ones that don't get initialized by the zero-arg constructor. So it looks like the RecvScreenForBrowser() call on line 358 is succeeding without initializing |screen|. And sure enough... > bool > ScreenManagerParent::RecvScreenForBrowser(PBrowserParent* aBrowser, > ScreenDetails* aRetVal, > bool* aSuccess) > { > *aSuccess = false; > > // Find the mWidget associated with the tabparent, and then return > // the nsIScreen it's on. > TabParent* tabParent = static_cast<TabParent*>(aBrowser); > nsCOMPtr<nsIWidget> widget = tabParent->GetWidget(); > if (!widget) { > return true; > } > > nsCOMPtr<nsIScreen> screen; > if (widget->GetNativeData(NS_NATIVE_WINDOW)) { > mScreenMgr->ScreenForNativeWidget(widget->GetNativeData(NS_NATIVE_WINDOW), > getter_AddRefs(screen)); > } > > NS_ENSURE_TRUE(screen, true); > > ScreenDetails details; > if (!ExtractScreenDetails(screen, details)) { > return true; > } > > *aRetVal = details; > *aSuccess = true; > return true; > } ... look at all those |true| values returned on error paths. It's definitely possible for |*aRetVal| to be left uninitialized. In fact, this whole function looks odd, but I guess the fact that |*aSuccess| is false on those paths somehow makes it ok? I.e. the process receiving the message won't inspect ScreenDetails if |*aSuccess| is false? (I see |*aSuccess| is part of the message.) Anyway, zeroing |*aRetVal| should do be enough to satisfy Valgrind.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8523634 -
Flags: review?(mconley)
It's kinda lame that IPDL doesn't initialize the POD members of its own structs though. Shouldn't we just fix IPDL to do this for us? (I guess as a followup)
Comment 3•10 years ago
|
||
Comment on attachment 8523634 [details] [diff] [review] Zero ScreenDetails outparam in RecvScreenForBrowser() so Valgrind doesn't complain Review of attachment 8523634 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the analysis, njn. (In reply to Nicholas Nethercote [:njn] from comment #0) > > ... look at all those |true| values returned on error paths. It's definitely > possible for |*aRetVal| to be left uninitialized. In fact, this whole > function > looks odd, but I guess the fact that |*aSuccess| is false on those paths > somehow makes it ok? I.e. the process receiving the message won't inspect > ScreenDetails if |*aSuccess| is false? (I see |*aSuccess| is part of the > message.) That's the idea, yes. We're returning true in this case, because a false forces the child process to crash (because as far as I can tell, the boolean return value for these IPC functions has less to do about the success of the operation, and more to do with the success of the communication). Correct - we do not inspect ScreenDetails if *aSuccess is false. > > Anyway, zeroing |*aRetVal| should do be enough to satisfy Valgrind. I'll take your word for it. :)
Attachment #8523634 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 4•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/28036503ada1
Comment 5•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/28036503ada1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•