Closed
Bug 981202
Opened 9 years ago
Closed 9 years ago
Compartment error in GetParamsFromSendMmsMessageRequest
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: gwagner, Assigned: bholley)
References
Details
(Keywords: regression)
Attachments
(2 files, 6 obsolete files)
971 bytes,
patch
|
hsinyi
:
review+
bzbarsky
:
feedback+
|
Details | Diff | Splinter Review |
2.01 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
STR: Send MMS with debug gecko on current trunk Program received signal SIGSEGV, Segmentation fault. 0xb570cb56 in js::ExclusiveContext::global (this=<optimized out>) at ../../../js/src/jscompartment.h:536 536 MOZ_ASSERT(compartment_, "Caller needs to enter a compartment first"); #0 0xb570cb56 in js::ExclusiveContext::global (this=<optimized out>) at ../../../js/src/jscompartment.h:536 #1 0xb570da76 in js::ExclusiveContext::global (this=<optimized out>) at ../../../js/src/jscompartment.h:538 #2 0xb57925ae in js::NewObjectWithClassProtoCommon (cxArg=0xb6bc3240, clasp=0xb642e724, protoArg=0x0, parentArg=0x0, allocKind=js::gc::FINALIZE_OBJECT0_BACKGROUND, newKind=js::GenericObject) at ../../../js/src/jsobj.cpp:1382 #3 0xb5710156 in NewObjectWithClassProto (newKind=js::GenericObject, allocKind=<optimized out>, parent=<optimized out>, proto=<optimized out>, clasp=<optimized out>, cx=<optimized out>) at ../../../js/src/jsobjinlines.h:906 #4 NewObjectWithClassProto (newKind=js::GenericObject, parent=<optimized out>, proto=<optimized out>, clasp=<optimized out>, cx=<optimized out>) at ../../../js/src/jsobjinlines.h:914 #5 JS_NewObject (cx=<optimized out>, jsclasp=<optimized out>, proto=..., parent=...) at ../../../js/src/jsapi.cpp:2572 #6 0xb4c11196 in GetParamsFromSendMmsMessageRequest (aParam=0xbee7d560, aRequest=..., aCx=0xb6bc3240) at ../../../../dom/mobilemessage/src/ipc/SmsParent.cpp:81 #7 mozilla::dom::mobilemessage::SmsRequestParent::DoRequest (this=0xa9974190, aRequest=...) at ../../../../dom/mobilemessage/src/ipc/SmsParent.cpp:485 #8 0xb45e2e5c in mozilla::dom::mobilemessage::PSmsParent::OnMessageReceived (this=0xa9970540, __msg=<optimized out>) at PSmsParent.cpp:523 [Child 1318] WARNING: Transparent content with displayports can be expensive.: file ../../../layout/base/nsDisplayList.cpp, line 1319 #9 0xb4568aa6 in mozilla::dom::PContentParent::OnMessageReceived (this=0xab128c00, __msg=...) at PContentParent.cpp:2013 #10 0xb450dc14 in mozilla::ipc::MessageChannel::DispatchAsyncMessage (this=0xab128c30, aMsg=...) at ../../../ipc/glue/MessageChannel.cpp:1142 #11 0xb4510c18 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne (this=0xab128c30) at ../../../ipc/glue/MessageChannel.cpp:1039 #12 0xb450d5e8 in DispatchToMethod<mozilla::ipc::MessageChannel, void (mozilla::ipc::MessageChannel::*)()> (method= (void (mozilla::ipc::MessageChannel::*)(mozilla::ipc::MessageChannel * const)) 0xb4510b85 <mozilla::ipc::MessageChannel::OnMaybeDequeueOne()>, obj=<optimized out>, arg=<optimized out>) at ../../../ipc/chromium/src/base/tuple.h:383 #13 RunnableMethod<mozilla::ipc::MessageChannel, void (mozilla::ipc::MessageChannel::*)(), Tuple0>::Run (this=<optimized out>) at ../../../ipc/chromium/src/base/task.h:307 #14 0xb450e098 in Run (this=<optimized out>) at ../../dist/include/mozilla/ipc/MessageChannel.h:383 #15 mozilla::ipc::MessageChannel::DequeueTask::Run (this=<optimized out>) at ../../dist/include/mozilla/ipc/MessageChannel.h:400 #16 0xb44ff6ac in MessageLoop::RunTask (this=0xb6b4e1a0, task=0xa8efd140) at ../../../ipc/chromium/src/base/message_loop.cc:344 #17 0xb44fffea in MessageLoop::DeferOrRunPendingTask (this=<optimized out>, pending_task=<optimized out>) at ../../../ipc/chromium/src/base/message_loop.cc:352 #18 0xb45010de in DoWork (this=<optimized out>) at ../../../ipc/chromium/src/base/message_loop.cc:452 #19 MessageLoop::DoWork (this=0xb6b4e1a0) at ../../../ipc/chromium/src/base/message_loop.cc:431 #20 0xb4511c6e in mozilla::ipc::DoWorkRunnable::Run (this=<optimized out>) at ../../../ipc/glue/MessagePump.cpp:228 #21 0xb4373b6a in ProcessNextEvent (result=0xbee7d807, mayWait=false, this=0xb6b34680) at ../../../xpcom/threads/nsThread.cpp:643 #22 nsThread::ProcessNextEvent (this=0xb6b34680, mayWait=<optimized out>, result=0xbee7d807) at ../../../xpcom/threads/nsThread.cpp:567 #23 0xb432dc70 in NS_ProcessNextEvent (thread=0xb6b34680, mayWait=<optimized out>) at ../../../xpcom/glue/nsThreadUtils.cpp:263 #24 0xb45120b8 in mozilla::ipc::MessagePump::Run (this=0xb6b01df0, aDelegate=0xb6b4e1a0) at ../../../ipc/glue/MessagePump.cpp:95 #25 0xb44ff7f2 in MessageLoop::RunInternal (this=0xb6b4e1a0) at ../../../ipc/chromium/src/base/message_loop.cc:226 #26 0xb44ff80a in RunHandler (this=0xb6b4e1a0) at ../../../ipc/chromium/src/base/message_loop.cc:219 #27 MessageLoop::Run (this=0xb6b4e1a0) at ../../../ipc/chromium/src/base/message_loop.cc:193 #28 0xb4a7b9aa in nsBaseAppShell::Run (this=0xb203a880) at ../../../widget/xpwidgets/nsBaseAppShell.cpp:164 #29 0xb532f306 in nsAppStartup::Run (this=0xb2244c70) at ../../../../toolkit/components/startup/nsAppStartup.cpp:276 #30 0xb530a610 in XREMain::XRE_mainRun (this=0xbee7d98c) at ../../../toolkit/xre/nsAppRunner.cpp:4008 #31 0xb530a806 in XREMain::XRE_main (this=0xbee7d98c, argc=<optimized out>, argv
Reporter | ||
Updated•9 years ago
|
blocking-b2g: --- → 1.3?
As of bug 981416 I also reproduce this on a build without B2G_DEBUG being set.
This looks like a regression from bug 979481. AutoSafeJSContext no longer enters a dummy compartment. It looks like you'll need to pick a compartment to enter in SmsRequestParent::DoRequest.
This is also reproducing on Inari running master.
Comment 5•9 years ago
|
||
Gregor, do you see this on 1.3? I saw this also but I tried to reproduce on 1.3 and I couldn't. From my observations I'd say it's 1.4 only. Gregor please ask 1.3? again if you think it's wrong, but even in your comment 0 you say it's on trunk.
blocking-b2g: 1.3? → 1.4?
Updated•9 years ago
|
Keywords: regression
Comment 6•9 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #3) > This looks like a regression from bug 979481. AutoSafeJSContext no longer > enters a dummy compartment. It looks like you'll need to pick a compartment > to enter in SmsRequestParent::DoRequest. I don't understand most of this but could someone check if the same issue happens for other less-experienced code? Also, NI Bevis to fix this for the MMS API.
Flags: needinfo?(btseng)
Comment 9•9 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #6) > (In reply to Bill McCloskey (:billm) from comment #3) > > This looks like a regression from bug 979481. AutoSafeJSContext no longer > > enters a dummy compartment. It looks like you'll need to pick a compartment > > to enter in SmsRequestParent::DoRequest. > > I don't understand most of this but could someone check if the same issue > happens for other less-experienced code? > > Also, NI Bevis to fix this for the MMS API. We run some device automation on m-c TBPL builds and I can correlate the bug 979741 patch being in the first TBPL build to fail.
Comment 10•9 years ago
|
||
(In reply to Zac C (:zac) from comment #9) > > I don't understand most of this but could someone check if the same issue > > happens for other less-experienced code? > > > > We run some device automation on m-c TBPL builds and I can correlate the bug > 979741 patch being in the first TBPL build to fail. don't know if you answered me here, but I really meant: "could someone that knows how a faulty code look like do a grep on the code base?" :)
Comment 11•9 years ago
|
||
take it first to clarify what's the root cause.
Assignee: nobody → btseng
Flags: needinfo?(btseng)
Comment 12•9 years ago
|
||
Bug 979481 was backedout as requested
![]() |
||
Comment 13•9 years ago
|
||
> could someone that knows how a faulty code look like do a grep on the code base Faulty code looks like this: AutoJSContext cx; CreateObjectsWithThisCompartment(cx); without a JSAutoCompartment between those two lines. And it's only faulty if the code involved is being run from the event loop directly, without JS already on the stack. Such code is inherently buggy, because it's creating objects in a bogus global; the chance that they will work as expected when passed to someone afterward is very slim. All bug 979481 did was make this code fail early at the object allocation site, instead of later when someone tries to work with the object and discovers it doesn't behave right.... That said, given that this is using xpidl and passing to an XPCOM service, there is no really obvious way to pass sane objects (e.g. ones that would end up as instanceof the right things in the callee). Bobby, what's a sane compartment for this code to enter? All of which begs the obvious question: how come there's no test coverage for this code? Bug 979481 was run through try, of course, and was green on landing.
Flags: needinfo?(bobbyholley)
Updated•9 years ago
|
Assignee: btseng → nobody
Comment 14•9 years ago
|
||
Looks like the regressing code has been backed out. If this still reproduces post backout of bug 979481, then reopen this bug.
Status: NEW → RESOLVED
blocking-b2g: 1.4? → 1.4+
Closed: 9 years ago
Resolution: --- → FIXED
![]() |
||
Comment 15•9 years ago
|
||
There's still a bug to be fixed here. See comment 13. It might need to be filed as a separate bug, but the code involved needs fixing. And actually, we need _another_ bug on getting test coverage for this code.
Flags: needinfo?(anygregor)
Reporter | ||
Comment 16•9 years ago
|
||
We have to fix the underlying problem here.
Status: RESOLVED → REOPENED
blocking-b2g: 1.4+ → ---
Flags: needinfo?(anygregor)
Resolution: FIXED → ---
Assignee | ||
Comment 17•9 years ago
|
||
This code is kind of ridiculous. It first creates a jsval representation of a SendMmsMessageRequest with GetParamsFromSendMmsMessageRequest. Then, it passes that to SmsIPCService::Send, which subsequently parses the jsval back into a SendMmsMessageRequest with GetSendMmsMessageRequestFromParams. Yuck.
Assignee | ||
Comment 18•9 years ago
|
||
Working up a patch.
Assignee: nobody → bobbyholley
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 19•9 years ago
|
||
Looks like Kyle has reviewed code here. Feel free to bounce the r? if it makes sense.
Attachment #8388693 -
Flags: review?(khuey)
Assignee | ||
Comment 20•9 years ago
|
||
Looks like Kyle has reviewed stuff in this file. Feel free to reassign.
Attachment #8388697 -
Flags: review?(khuey)
![]() |
||
Comment 21•9 years ago
|
||
Comment on attachment 8388693 [details] [diff] [review] Stop round-tripping between C++ and JS representations of SendMmsMessageRequest. v1 Don't we need to call SendWithReq somewhere?
Assignee | ||
Comment 22•9 years ago
|
||
Duh. Good catch Boris.
Attachment #8388693 -
Attachment is obsolete: true
Attachment #8388697 -
Attachment is obsolete: true
Attachment #8388693 -
Flags: review?(khuey)
Attachment #8388697 -
Flags: review?(khuey)
Attachment #8388708 -
Flags: review?(khuey)
Assignee | ||
Comment 23•9 years ago
|
||
Attachment #8388710 -
Flags: review?(khuey)
Have you tested these patches?
Assignee | ||
Comment 25•9 years ago
|
||
Comment on attachment 8388710 [details] [diff] [review] Stop round-tripping between C++ and JS representations of SendMmsMessageRequest. v2 Sorry for the extra churn - bugzilla is being a PITA.
Attachment #8388710 -
Attachment is obsolete: true
Attachment #8388710 -
Flags: review?(khuey)
Assignee | ||
Comment 26•9 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #24) > Have you tested these patches? I have not, no. I can certainly push it to try, but I'd rather do that after addressing any review comments. I'm fine with "r+ as long as it passes try".
I'm kinda concerned about whether SMS works after the patches ... I don't think it's well tested, even on try.
![]() |
||
Comment 28•9 years ago
|
||
Given that bug 979481 was green on try before landing, I think I can confidently say this codepath is NOT exercised on try. :(
![]() |
||
Comment 29•9 years ago
|
||
Can one exercise this code without a device? I assume no, right?
Comment 30•9 years ago
|
||
ignore this, testing. apologies....
Assignee | ||
Comment 31•9 years ago
|
||
jsmith, can you find someone to test this patch?
Flags: needinfo?(jsmith)
Comment 32•9 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #31) > jsmith, can you find someone to test this patch? If someone can spin a custom gecko image & host it somewhere, then yeah, I can get someone to test this.
Flags: needinfo?(jsmith)
Assignee | ||
Comment 33•9 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #32) > If someone can spin a custom gecko image & host it somewhere, then yeah, I > can get someone to test this. Can I do that with try?
Reporter | ||
Comment 34•9 years ago
|
||
I applied the patch but mms don't work any more. Its a silent failure. Nothing in logcat :(
Comment 36•9 years ago
|
||
Patrick, you implemented this before. May you please look into this bugs? Thanks.
Flags: needinfo?(btseng) → needinfo?(pwang)
Comment 37•9 years ago
|
||
This one line patch runs test_mmsmessage_attachments.js in both OOP and in-process mode. So it should pass either with or without bz's patch. Try without bz's patch: https://tbpl.mozilla.org/?tree=Try&rev=39953510fb7d
Attachment #8389191 -
Flags: review?(htsai)
Attachment #8389191 -
Flags: feedback?(bzbarsky)
Comment 38•9 years ago
|
||
Depending on bug 970804 for that oop flag in marionette manifest.
Depends on: 970804
Assignee | ||
Comment 39•9 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #37) > Try without bz's patch: https://tbpl.mozilla.org/?tree=Try&rev=39953510fb7d They're my patches. ;-) Patrick, can you have a look at the patch I attached to this bug? If we go with that strategy, it should remove the bustage caused by bug 979481.
Comment 40•9 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #39) > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #37) > > Try without bz's patch: https://tbpl.mozilla.org/?tree=Try&rev=39953510fb7d > > They're my patches. ;-) > > Patrick, can you have a look at the patch I attached to this bug? If we go > with that strategy, it should remove the bustage caused by bug 979481. Actually it doesn't seem work. The test case patch I attached fails when your patch is also applied.
Flags: needinfo?(pwang)
Assignee | ||
Comment 41•9 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #40) > Actually it doesn't seem work. The test case patch I attached fails when > your patch is also applied. Yes, that's what Gregor reported in comment 34. That's why it would be nice if someone who works on this code could look at the patch and un-bust it, or explain why it's wrong.
Flags: needinfo?(pwang)
Comment 42•9 years ago
|
||
Comment on attachment 8388708 [details] [diff] [review] Stop round-tripping between C++ and JS representations of SendMmsMessageRequest. v2 Review of attachment 8388708 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/ipc/SmsParent.cpp @@ +369,5 @@ > req.silent(), this); > } > break; > case SendMessageRequest::TSendMmsMessageRequest: { > nsCOMPtr<nsIMmsService> mmsService = do_GetService(MMS_SERVICE_CONTRACTID); On parent side, this actually gets a ril mms service[1], which is written in js[2]. I think that is why this doesn't work. [1] http://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/SmsServicesFactory.cpp#60 [2] http://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/MmsService.manifest#3
Updated•9 years ago
|
Flags: needinfo?(pwang)
Attachment #8388708 -
Flags: review?(khuey)
![]() |
||
Updated•9 years ago
|
Attachment #8389191 -
Flags: feedback?(bzbarsky) → feedback+
Assignee | ||
Comment 43•9 years ago
|
||
(In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #42) > On parent side, this actually gets a ril mms service[1], which is written in > js[2]. I think that is why this doesn't work. Ugh, /me curses the non-greppability of JS-Implemented components. Ok, let's just do something dumb, then.
Assignee | ||
Comment 44•9 years ago
|
||
Gregor, does this pass a smoke test?
Attachment #8388619 -
Attachment is obsolete: true
Attachment #8388708 -
Attachment is obsolete: true
Attachment #8389936 -
Flags: feedback?
Assignee | ||
Updated•9 years ago
|
Attachment #8389936 -
Flags: feedback? → feedback?(anygregor)
Reporter | ||
Comment 45•9 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #44) > Created attachment 8389936 [details] [diff] [review] > Create js-ified param representations in the System Junk Scope. v1 > > Gregor, does this pass a smoke test? Sorry for the delay but I can't reproduce this issue any more on current trunk :(
Updated•9 years ago
|
Attachment #8389191 -
Flags: review?(htsai) → review+
Assignee | ||
Comment 46•9 years ago
|
||
(In reply to Gregor Wagner [:gwagner] from comment #45) > Sorry for the delay but I can't reproduce this issue any more on current > trunk :( Did you apply the patches from bug 979481, which were backed out because of this crash?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(anygregor)
Reporter | ||
Comment 47•9 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #46) > (In reply to Gregor Wagner [:gwagner] from comment #45) > > Sorry for the delay but I can't reproduce this issue any more on current > > trunk :( > > Did you apply the patches from bug 979481, which were backed out because of > this crash? I did not :( I am in MV today without my device but I can test tomorrow.
Flags: needinfo?(anygregor)
Assignee | ||
Comment 48•9 years ago
|
||
(In reply to Gregor Wagner [:gwagner] from comment #47) > I did not :( I am in MV today without my device but I can test tomorrow. Awesome! Thanks :-)
Reporter | ||
Comment 49•9 years ago
|
||
Vicamo, can you test this with a debug build? My nexus 4 is broken right now.
Flags: needinfo?(vyang)
Comment 50•9 years ago
|
||
Hi, can't apply patches in bug 979481 on to m-c tip. Need rebase. NI Bobby in that bug.
Flags: needinfo?(vyang)
Comment 51•9 years ago
|
||
Comment on attachment 8389936 [details] [diff] [review] Create js-ified param representations in the System Junk Scope. v1 Review of attachment 8389936 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/ipc/SmsParent.cpp @@ +484,2 @@ > AutoJSContext cx; > + JSAutoCompartment ac(cx, xpc::GetJunkScope()); Name space 'xpc' is undefined here. I add a line '#include "xpcpublic.h"' to fix this.
Attachment #8389936 -
Flags: feedback+
Assignee | ||
Comment 52•9 years ago
|
||
Attachment #8389936 -
Attachment is obsolete: true
Attachment #8389936 -
Flags: feedback?(anygregor)
Attachment #8396326 -
Flags: review?(anygregor)
Reporter | ||
Updated•9 years ago
|
Attachment #8396326 -
Flags: review?(anygregor) → review?(mrbkap)
Updated•9 years ago
|
Attachment #8396326 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 53•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/77c860332b5e
Comment 54•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/77c860332b5e
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•