981202 - Compartment error in GetParamsFromSendMmsMessageRequest

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gregor Wagner [:gwagner]]

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

Comment 2

[Alexandre LISSY :gerard-majax]

As of bug 981416 I also reproduce this on a build without B2G_DEBUG being set.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 4

[Alexandre LISSY :gerard-majax]

This is also reproducing on Inari running master.

Comment 5

[Julien Wajsberg [:julienw] (PTO → 29th)]

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.

Comment 6

[Julien Wajsberg [:julienw] (PTO → 29th)]

(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.

Comment 9

[Zac C (:zac)]

(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

[Julien Wajsberg [:julienw] (PTO → 29th)]

(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

[Bevis Tseng[:bevis][:btseng](Exited)]

take it first to clarify what's the root cause.

Comment 12

[Carsten Book [:Tomcat]]

Bug 979481 was backedout  as requested

Comment 13

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 14

[Jason Smith [:jsmith]]

Looks like the regressing code has been backed out. If this still reproduces post backout of bug 979481, then reopen this bug.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

[Gregor Wagner [:gwagner]]

We have to fix the underlying problem here.

Comment 17

[Bobby Holley (:bholley)]

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.

Comment 18

[Bobby Holley (:bholley)]

Working up a patch.

Comment 19

[Bobby Holley (:bholley)]

Attachment: Stop round-tripping between C++ and JS representations of SendMmsMessageRequest. v1

Looks like Kyle has reviewed code here. Feel free to bounce the r? if it
makes sense.

Comment 20

[Bobby Holley (:bholley)]

Attachment: 0001-Bug-981202-Stop-round-tripping-between-C-and-JS-repr.patch

Looks like Kyle has reviewed stuff in this file. Feel free to reassign.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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?

Comment 22

[Bobby Holley (:bholley)]

Attachment: Stop round-tripping between C++ and JS representations of SendMmsMessageRequest. v2

Duh. Good catch Boris.

Comment 23

[Bobby Holley (:bholley)]

Attachment: Stop round-tripping between C++ and JS representations of SendMmsMessageRequest. v2

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Have you tested these patches?

Comment 25

[Bobby Holley (:bholley)]

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.

Comment 26

[Bobby Holley (:bholley)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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".

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm kinda concerned about whether SMS works after the patches ... I don't think it's well tested, even on try.

Comment 28

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

Can one exercise this code without a device?  I assume no, right?

Comment 30

[Adrian J Fernandez [:Aj]]

ignore this, testing. apologies....

Comment 31

[Bobby Holley (:bholley)]

jsmith, can you find someone to test this patch?

Comment 32

[Jason Smith [: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.

Comment 33

[Bobby Holley (:bholley)]

(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?

Comment 34

[Gregor Wagner [:gwagner]]

I applied the patch but mms don't work any more. Its a silent failure. Nothing in logcat :(

Comment 35

[Julien Wajsberg [:julienw] (PTO → 29th)]

Hey Bevis, I think you should get involved here.

Comment 36

[Ken Chang[:kenkai]]

Patrick, you implemented this before. May you please look into this bugs? Thanks.

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2: test cases

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

Comment 38

[Vicamo Yang [:vicamo][:vyang]]

Depending on bug 970804 for that oop flag in marionette manifest.

Comment 39

[Bobby Holley (:bholley)]

(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

[Vicamo Yang [:vicamo][:vyang]]

(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.

Comment 41

[Bobby Holley (:bholley)]

(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.

Comment 42

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

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

Comment 43

[Bobby Holley (:bholley)]

(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.

Comment 44

[Bobby Holley (:bholley)]

Attachment: Create js-ified param representations in the System Junk Scope. v1

Gregor, does this pass a smoke test?

Comment 45

[Gregor Wagner [:gwagner]]

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

Comment 46

[Bobby Holley (:bholley)]

(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?

Comment 47

[Gregor Wagner [:gwagner]]

(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.

Comment 48

[Bobby Holley (:bholley)]

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

Comment 49

[Gregor Wagner [:gwagner]]

Vicamo, can you test this with a debug build? My nexus 4 is broken right now.

Comment 50

[Vicamo Yang [:vicamo][:vyang]]

Hi, can't apply patches in bug 979481 on to m-c tip.  Need rebase.  NI Bobby in that bug.

Comment 51

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 52

[Bobby Holley (:bholley)]

Attachment: Create js-ified param representations in the System Junk Scope. v2 f=vicamo

Comment 53

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77c860332b5e

Comment 54

[[Back August 16] Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/77c860332b5e