Closed Bug 981202 Opened 9 years ago Closed 9 years ago

Compartment error in GetParamsFromSendMmsMessageRequest

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla31

People

(Reporter: gwagner, Assigned: bholley)

References

Details

(Keywords: regression)

Attachments

(2 files, 6 obsolete files)

part 2: test cases
971 bytes, patch
hsinyi
: review+
bzbarsky
: feedback+
Details | Diff | Splinter Review
Create js-ified param representations in the System Junk Scope. v2 f=vicamo
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
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.
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?
(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)
Blocks: 979481
(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.
(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?" :)
take it first to clarify what's the root cause.
Assignee: nobody → btseng
Flags: needinfo?(btseng)
Bug 979481 was backedout  as requested
> 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)
Assignee: btseng → nobody
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
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)
We have to fix the underlying problem here.
Status: RESOLVED → REOPENED
blocking-b2g: 1.4+ → ---
Flags: needinfo?(anygregor)
Resolution: FIXED → ---
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.
Working up a patch.
Assignee: nobody → bobbyholley
Flags: needinfo?(bobbyholley)
Looks like Kyle has reviewed code here. Feel free to bounce the r? if it
makes sense.
Attachment #8388693 - Flags: review?(khuey)
Looks like Kyle has reviewed stuff in this file. Feel free to reassign.
Attachment #8388697 - Flags: review?(khuey)
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?
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)
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)
(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.
Given that bug 979481 was green on try before landing, I think I can confidently say this codepath is NOT exercised on try.  :(
Can one exercise this code without a device?  I assume no, right?
ignore this, testing. apologies....
jsmith, can you find someone to test this patch?
Flags: needinfo?(jsmith)
(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)
(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?
I applied the patch but mms don't work any more. Its a silent failure. Nothing in logcat :(
Hey Bevis, I think you should get involved here.
Flags: needinfo?(btseng)
Patrick, you implemented this before. May you please look into this bugs? Thanks.
Flags: needinfo?(btseng) → needinfo?(pwang)
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)
Depending on bug 970804 for that oop flag in marionette manifest.
Depends on: 970804
(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.
(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)
(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 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
Flags: needinfo?(pwang)
Attachment #8389191 - Flags: feedback?(bzbarsky) → feedback+
(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.
Gregor, does this pass a smoke test?
Attachment #8388619 - Attachment is obsolete: true
Attachment #8388708 - Attachment is obsolete: true
Attachment #8389936 - Flags: feedback?
Attachment #8389936 - Flags: feedback? → feedback?(anygregor)
(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 :(
Attachment #8389191 - Flags: review?(htsai) → review+
(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?
Flags: needinfo?(anygregor)
(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)
(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 :-)
Depends on: 985775
Vicamo, can you test this with a debug build? My nexus 4 is broken right now.
Flags: needinfo?(vyang)
Hi, can't apply patches in bug 979481 on to m-c tip.  Need rebase.  NI Bobby in that bug.
Flags: needinfo?(vyang)
Depends on: 986572
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+

        Attachment #8389936 -
        Attachment is obsolete: true

        Attachment #8389936 -
        Flags: feedback?(anygregor)

        Attachment #8396326 -
        Flags: review?(anygregor)

    
  

        Attachment #8396326 -
        Flags: review?(anygregor) → review?(mrbkap)

        Attachment #8396326 -
        Flags: review?(mrbkap) → review+

    
    

    
  
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.