775997 - Message app crashes when run OOP

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P1)

Description

[Dave Hylands [:dhylands]]

While trying to launch the Message App as OOP, I observe the following:

[Parent 833] ###!!! ASSERTION: Got an invalid request id or it has been already deleted!: 'mRequests.Count() > aRequestId && mRequests[aRequestId]', file /home/work/B2G-otoro/gecko-git/dom/sms/src/SmsRequestManager.cpp, line 96
[Parent 833] ###!!! ASSERTION: nsVoidArray::FastElementAt: index out of range: '0 <= aIndex && aIndex < Count()', file ../../../dist/include/nsVoidArray.h, line 48

Program received signal SIGSEGV, Segmentation fault.
0x40b5f98a in nsCOMArray<nsIDOMMozSmsRequest>::operator[] (this=0x17f489c, aIndex=4) at ../../../dist/include/nsCOMArray.h:154
154	    }
(gdb) bt
#0  0x40b5f98a in nsCOMArray<nsIDOMMozSmsRequest>::operator[] (this=0x17f489c, aIndex=4) at ../../../dist/include/nsCOMArray.h:154
#1  0x40b5f9e0 in mozilla::dom::sms::SmsRequestManager::GetRequest (this=0x17f4890, aRequestId=0) at /home/work/B2G-otoro/gecko-git/dom/sms/src/SmsRequestManager.cpp:100
#2  0x40b5fec4 in mozilla::dom::sms::SmsRequestManager::NotifyNoMessageInList (this=0x4, aRequestId=4) at /home/work/B2G-otoro/gecko-git/dom/sms/src/SmsRequestManager.cpp:167
#3  0x4122eeda in nsXPTCStubBase::Stub10() () from /home/work/B2G-otoro/objdir-gecko/dist/bin/libxul.so
#4  0x4122eeda in nsXPTCStubBase::Stub10() () from /home/work/B2G-otoro/objdir-gecko/dist/bin/libxul.so
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

Comment 1

[Dave Hylands [:dhylands]]

All core apps need to run OOP

Comment 2

[Dave Hylands [:dhylands]]

A little bit of further investigation.

The call to CreateRequest is occuring on the plugin-container process.

The NotifyNoMessageInList callback is occuring in the parent b2g process, and it's trying to access the request from the other process, which can't work.

Comment 3

[Dave Hylands [:dhylands]]

Here's the function flow (lines prefixed by Content: are called from the content (child) process, and lines prefixed by Main: are called from the main b2g (parent) process):

Content: calls SmsManager::GetMessages
Content: SmsManager::GetMessages calls SmsRequestManager::CreateRequest
Content: SmsManager::GetMessages calls SmsIPCService::CreateMessageList passing in requestId created above
Content: SmsIPCService::CreateMessageList calls PSmsChild::SendCreateMessageList
Content: PSmsChild::SendCreateMessageList sends message to parent

Main:    SmsParent::RecvCreateMessageList calls SmsDatabaseService.js createMessageList
Main:    SmsDatabaseService.js createMessageList calls notifyNoMessageInList
Main:    SmsDatabaseService.js notifyNoMessageInList calls SmsRequestManager::NotifyNoMessageInList
Main:    SmsRequestManager::NotifyNoMessageInList calls GetRequest, but the request was allocated in the Content process.

It looks like the NotifyNoMessageInList should get sent back to the content process.

I see SmsChild::RecvNotifyRequestNoMessageInList, but nobody seems to call the SmsParent::SendNotifyRequestNoMessageInList in the parent.

Comment 4

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

Attachment: WIP

implement IPC protocol for SmsRequestManager

Comment 5

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

Attachment: WIP2

1. Add process id to SmsRequestManager.notify*.
2. Respect process id in SmsDatabaseService & RadioInterfaceLayer.

Comment 6

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

Comment on attachment 653667 [details] [diff] [review]
WIP2

Review of attachment 653667 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/sms/src/SmsServicesFactory.cpp
@@ +55,5 @@
> +SmsServicesFactory::CreateSmsRequestManager()
> +{
> +  nsCOMPtr<nsISmsRequestManager> smsRequestManager;
> +
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {

We want nsISmsRequestManager created as:

        | Type_Default      | Type_Content
------------------------------------------------
OOP     | SmsIPCService     | SmsRequestManager
Non-OOP | SmsRequestManager | N/A

However, we don't know whether the app is running in OOP or not. Things go worse when there are multiple applications that uses WebSMS but running in different modes.

This patches fails apps using WebSMS API but not being launched in OOP mode. How do we fix this?

Comment 7

[Mounir Lamouri (:mounir)]

Comment on attachment 653667 [details] [diff] [review]
WIP2

Review of attachment 653667 [details] [diff] [review]:
-----------------------------------------------------------------

f+. However, I'm not sure I understood the issues you said you had.

::: dom/sms/src/SmsRequestManager.h
@@ +15,5 @@
>  namespace mozilla {
>  namespace dom {
>  namespace sms {
>  
> +class SmsRequestManager MOZ_FINAL : public nsISmsRequestManager

You are not changing anything here, are you?

::: dom/sms/src/SmsServicesFactory.cpp
@@ +55,5 @@
> +SmsServicesFactory::CreateSmsRequestManager()
> +{
> +  nsCOMPtr<nsISmsRequestManager> smsRequestManager;
> +
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {

I don't understand what you are trying to say in that comment.

Also, please do:
if (XRE_GetProcessType() == GeckoProcessType_Content) {
  smsRequestManager = new SmsRequestManager();
} else {
  smsRequestManager = new SmsIPCService();
}

And put a comment explaining that this is the opposite pattern than the other two services: we use the ipc service when in the parent process, to talk with the child processes.

::: dom/sms/src/ipc/SmsIPCService.cpp
@@ +160,5 @@
> +SmsIPCService::CreateRequest(nsIDOMMozSmsManager* aManager,
> +                             nsIDOMMozSmsRequest** aRequest,
> +                             PRInt32* aRequestId)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;

I don't really like the idea of having something in the interface but not usable in a specific case. However, I don't have a better idea.

At least, put a comment explaining that the content process is handling the requests so the parent process can't create any.

@@ +167,5 @@
> +NS_IMETHODIMP
> +SmsIPCService::AddRequest(nsIDOMMozSmsRequest* aRequest,
> +                          PRInt32* aRequestId)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;

ditto

Comment 8

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

(In reply to Mounir Lamouri (:mounir) from comment #7)
> Comment on attachment 653667 [details] [diff] [review]
> WIP2
> 
> Review of attachment 653667 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f+. However, I'm not sure I understood the issues you said you had.
> 
> ::: dom/sms/src/SmsRequestManager.h
> @@ +15,5 @@
> >  namespace mozilla {
> >  namespace dom {
> >  namespace sms {
> >  
> > +class SmsRequestManager MOZ_FINAL : public nsISmsRequestManager
> 
> You are not changing anything here, are you?

Let SmsRequestManager publicly inherits nsISmsRequestManager for we'll need it in SmsServiceFactory.

> ::: dom/sms/src/SmsServicesFactory.cpp
> @@ +55,5 @@
> > +SmsServicesFactory::CreateSmsRequestManager()
> > +{
> > +  nsCOMPtr<nsISmsRequestManager> smsRequestManager;
> > +
> > +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> 
> I don't understand what you are trying to say in that comment.
> 
> Also, please do:
> if (XRE_GetProcessType() == GeckoProcessType_Content) {
>   smsRequestManager = new SmsRequestManager();
> } else {
>   smsRequestManager = new SmsIPCService();
> }
> 
> And put a comment explaining that this is the opposite pattern than the
> other two services: we use the ipc service when in the parent process, to
> talk with the child processes.

With application running in OOP mode, creating nsISmsRequestManager by checking `XRE_GetProcessType() == GeckoProcessType_Content` does work fine. But when it comes to non-OOP mode, there is only one process, and its type will be GeckoProcessType_Default. As the result, it will create a SmsIPCService and there will be no SmsRequestManager instance created anyother place.

On the contrary, if we check `XRE_GetProcessType() == GeckoProcessType_Default` instead, we still have the same problem as described above. In fact, chrome process side must always create the same nsISmsRequestManager instance as what we have in nsISmsService and nsISmsDatabaseService to ensure co-existence of OOP and non-OOP API access. However, the content processes depend on different protocols, IPC or non-IPC, in different modes, and the chrome process has do the same to ensure they speak the same language. So whether we check GeckoProcessType_Default or GeckoProcessType_Content doesn't really matter here. They just have the same problem.

> ::: dom/sms/src/ipc/SmsIPCService.cpp
> @@ +160,5 @@
> > +SmsIPCService::CreateRequest(nsIDOMMozSmsManager* aManager,
> > +                             nsIDOMMozSmsRequest** aRequest,
> > +                             PRInt32* aRequestId)
> > +{
> > +  return NS_ERROR_NOT_IMPLEMENTED;
> 
> I don't really like the idea of having something in the interface but not
> usable in a specific case. However, I don't have a better idea.
> 
> At least, put a comment explaining that the content process is handling the
> requests so the parent process can't create any.

Ok, that will be more clear.

Comment 9

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

Change LOE tag to medium for it seems to be more complex than I thought.

Comment 10

[Mounir Lamouri (:mounir)]

Hmmm, I don't see how we could solve that by using a service... Maybe we could do like my Android implementation is doing and have the caller use IPC or not, depending on whether it is in a child process or not. But, that sounds harder to do from JS.

Chris, do you happen to have an idea in how to solve this cleanly?

Comment 11

[Philipp von Weitershausen [:philikon]]

(In reply to Mounir Lamouri (:mounir) from comment #10)
> Hmmm, I don't see how we could solve that by using a service... Maybe we
> could do like my Android implementation is doing and have the caller use IPC
> or not, depending on whether it is in a child process or not. But, that
> sounds harder to do from JS.

Can we expose a helper to JS that will return the right object, depending on whether we're OOP or not? Perhaps we retrieve the SmsRequestManager from the SmsService, e.g.

  gSmsService.QueryInterface(Ci.nsIInterfaceRequestor)
             .getInterface(Ci.nsISmsRequestService);

Just a stab in the dark.

Comment 12

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

(In reply to Philipp von Weitershausen [:philikon] from comment #11)
> (In reply to Mounir Lamouri (:mounir) from comment #10)
> > Hmmm, I don't see how we could solve that by using a service... Maybe we
> > could do like my Android implementation is doing and have the caller use IPC
> > or not, depending on whether it is in a child process or not. But, that
> > sounds harder to do from JS.
> 
> Can we expose a helper to JS that will return the right object, depending on
> whether we're OOP or not? Perhaps we retrieve the SmsRequestManager from the
> SmsService, e.g.
> 
>   gSmsService.QueryInterface(Ci.nsIInterfaceRequestor)
>              .getInterface(Ci.nsISmsRequestService);
> 
> Just a stab in the dark.

Like what we discussed yesterday, this solves the problem we have now with ONE application. If you get several applications that use WebSMS API, and some of them are running in OOP while others don't, then this trick fails.

In fact, we now realize that SmsRequestManager should always be the same type in chrome process side. Moving SmsRequestManager into chrome process like SmsService can be a solution, but there is actually no much things to be moved into chrome process. Besides, there will be a lot of new observer events, broadcasts, inefficient things.

Comment 13

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

Attachment: Part 1: IDL changes

1. Remove SmsRequestManager, move its functionalities to nsIDOMSmsRequest. The AddRequest/CreateRequest calls of the original SmsRequestManager will also be amended into nsIDOMSmsRequest.enqueue().
2. Create new IPDL protocol PSmsRequest to allow encapsulating request IPC calls.
3. Let nsIDOMSmsRequest implements nsISupports only. This interface is going to be wrapped in chrome process, so we won't want it to implement nsIDOMDOMRequest.

Comment 14

[Philipp von Weitershausen [:philikon]]

Comment on attachment 656415 [details] [diff] [review]
Part 1: IDL changes

Review of attachment 656415 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/sms/interfaces/nsIDOMSmsRequest.idl
@@ +6,5 @@
>  
> +interface nsIDOMMozSmsMessage;
> +
> +[scriptable, builtinclass, uuid(97067327-64b9-4e26-848b-59e443c55db9)]
> +interface nsIDOMMozSmsRequest : nsISupports

Repurposing nsIDOMMozSmsRequest is dangerous because it's an interface that's exposed to content. Instead of morphing nsIDOMMozSmsRequest, I suggest we leave it alone and create a *new*, chrome-only interface (e.g. nsISmsRequest) for the functionality. In fact, it seems more logical to morph nsISmsRequestManager into nsISmsRequest since that's basically what's happening here.

I can't say anything about the IPC magic here. :/

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 656415 [details] [diff] [review]
Part 1: IDL changes

IPC magic looks great!  Deferring reviews of IDL magic to philikon/mounir.

Comment 16

[Mounir Lamouri (:mounir)]

Comment on attachment 656415 [details] [diff] [review]
Part 1: IDL changes

Review of attachment 656415 [details] [diff] [review]:
-----------------------------------------------------------------

nsIDOMSmsRequest changes a wrong. This interface is exposed to the DOM and should stay as it was. You could add a ISmsRequest interface that would have those methods if you want/need.

Comment 17

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

Attachment: Part A: inherit nsISmsRequest : WIP

This is the first patch that tries to rewrite SMS request notification code. These patches are meant to pass build process and ensure each of them still retains normal SMS functionality. I'll integrate them after complete done.

Comment 18

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

Attachment: Part B: move SmsRequest creating, storage into itself : WIP

Comment 19

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

Attachment: Part C: move event emittion into SmsRequest : WIP

Comment 20

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

Attachment: Part D: really remove SmsRequestManager

This patch passes build process, b2g launches as usual, but the SMS API has never any response from gecko. In details, line 301 in SmsManager.cpp:

> smsDBService->CreateMessageList(filter, aReverse,
>                                 static_cast<SmsRequest*>(*aRequest));

never reaches corresponding javascript implementation in ril/SmsDatabaseService.js. Still don't know why.

Comment 21

[Mounir Lamouri (:mounir)]

Comment on attachment 658125 [details] [diff] [review]
Part D: really remove SmsRequestManager

Review of attachment 658125 [details] [diff] [review]:
-----------------------------------------------------------------

The patch generally looks good but I'm not sure what I had to look at exactly so I just had a quick overview.
BTW, what did you decide to do for nsIDOMSmsRequest (regarding my comment on your first patch)?

::: dom/sms/src/SmsManager.cpp
@@ +157,5 @@
>  
>    nsDependentJSString number;
>    number.init(aCx, aNumber);
>  
> +  smsService->Send(number, aMessage, static_cast<SmsRequest*>(request.get()));

It would be nice to prevent that. Can't SmsRequest::Create() return a SmsRequest* instead of a nsIDOMMozSmsRequest. SmsRequest inheriting from nsIDOMMozSmsRequest, the casting the other way around would be better.

Comment 22

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

(In reply to Mounir Lamouri (:mounir) from comment #21)
> BTW, what did you decide to do for nsIDOMSmsRequest (regarding my comment on
> your first patch)?

I'll keep nsIDOMSmsRequest and create a new nsISmsRequest interface as you both suggested. SmsRequest will inherit both of them as shown in Part A.

Comment 23

[Mounir Lamouri (:mounir)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #22)
> (In reply to Mounir Lamouri (:mounir) from comment #21)
> > BTW, what did you decide to do for nsIDOMSmsRequest (regarding my comment on
> > your first patch)?
> 
> I'll keep nsIDOMSmsRequest and create a new nsISmsRequest interface as you
> both suggested. SmsRequest will inherit both of them as shown in Part A.

Great :)

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hi Vicamo, how is this work coming along?

Comment 25

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #24)
> Hi Vicamo, how is this work coming along?

I just had other blocking-basecamp issues in review last evening. Rebasing my previous patches A-D and found a segfault in B with latest gecko. Still working on it.

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hi Vicamo, is there any work here that I can help out with?

Comment 27

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #26)
> Hi Vicamo, is there any work here that I can help out with?

Hi Chris, I haven't been working on this since my last comment a week ago. I think the status remains segfault in patch B and no corresponding js method invoked as described in comment #20. For the second problem, I expect a debug print in ril/SmsDatabaseManager.js, function createMessageList() shows up but doesn't.

Comment 28

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

I have to drop this issue because I'm running out of time and probably not capable of solving it now. :(

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #28)
> I have to drop this issue because I'm running out of time and probably not
> capable of solving it now. :(

Note, the work here isn't a new feature, it's fixing bugs in an existing feature.  So we don't need this in by the feature freeze.  (Although I'd really like to get it in by then.)

Comment 31

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part D: really remove SmsRequestManager (rebased)

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm pretty sure I'm seeing the same symptoms as comment 27; no outgoing or incoming SMS.

But I'm double checking things are still OK in a build without these patches applied ...

Comment 33

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yeah, looks good in my base gaia/gecko.

Comment 34

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I guess not surprisingly, I see the same kind of bugs when the SMS app runs in-process, but with these patches applied.

Comment 35

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

So with all the code in-process, we seem to be receiving and processing messages, and I see |id = gSmsDatabaseService.saveReceivedMessage()| assigning a valid ID.  We post a notification to the tray.  But when I tap the notification to open the sms app, it doesn't seem to be able to enumerate the database.

Comment 36

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

While context-switching to work on another bug, I rebuilt without these patches applied.  All the messages I sent in comment 35, but wasn't able to see in the SMS app, I was in my build without the patches.  So we're still saving successfully to the DB; it's just enumerating the DB that's buggy.

Comment 37

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Interestingly, no errors or assertions in a debug build.

Comment 38

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

If I change GetMessages() to do

  rv = smsDBService->CreateMessageList(filter, aReverse, static_cast<SmsRequest*>(*aRequest));
  if (NS_FAILED(rv)) {
    // If we failed to create the request, there's no way for the SMS
    // client to notice that the request failed.
    NS_ERROR("CreateMessageList() failed!");
    return rv;

then I see

I/Gecko   (15229): [Parent 15229] ###!!! ASSERTION: CreateMessageList() failed!: 'Error', file /home/cjones/mozilla/inbound/dom/sms/src/SmsManager.cpp, line 307
E/GeckoConsole(15229): [JavaScript Error: "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMMozSmsManager.getMessages]" {file: "app://sms.gaiamobile.org/js/sms.js" line: 150}]

hmmmm ....

Comment 39

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

In a debug/no-opt build, I'm not able to step through to the point where NS_ERROR_FAILURE is meaningfully returned by something.  I see this in gdb

Breakpoint 1, mozilla::dom::sms::SmsManager::GetMessages (this=0x4a577100, aFilter=0x0, aReverse=true, aRequest=0xbedf41b0) at /home/cjones/mozilla/inbound/dom/sms/src/SmsManager.cpp:307
(gdb) s
nsCOMPtr<nsISmsService>::operator-> (this=0xbedf40a8) at ../../../dist/include/nsCOMPtr.h:779
(gdb) 
(gdb) 
(gdb) 
nsCOMPtr<nsIRDFDelegateFactory>::get (this=0xbedf40a8) at ../../dist/include/nsTString.h:76
(gdb) 
(gdb) 
nsCOMPtr<nsISmsService>::operator-> (this=0xbedf40a8) at ../../../dist/include/nsCOMPtr.h:783
(gdb) 
nsCOMPtr<nsISupports>::operator nsISupports* (this=0xbedf40a8) at ../../dist/include/nsCOMPtr.h:1072
(gdb) 
(gdb) 
nsCOMPtr<nsIRDFDelegateFactory>::get (this=0xbedf40b0) at ../../dist/include/nsTString.h:76
(gdb) 
(gdb) 
nsCOMPtr<nsISupports>::operator nsISupports* (this=0xbedf40b0) at ../../dist/include/nsCOMPtr.h:1083
(gdb) 
nsCOMPtr<nsISupports>::operator nsISupports* (this=0xbedf40b0) at ../../dist/include/nsCOMPtr.h:1072
(gdb) 
(gdb) 
nsCOMPtr<nsIRDFDelegateFactory>::get (this=0xbedf40ac) at ../../dist/include/nsTString.h:76
(gdb) 
(gdb) 
nsCOMPtr<nsISupports>::operator nsISupports* (this=0xbedf40ac) at ../../dist/include/nsCOMPtr.h:1083
(gdb) 
mozilla::dom::sms::SmsManager::GetMessages (this=0x4a577100, aFilter=0x0, aReverse=true, aRequest=0xbedf41b0) at /home/cjones/mozilla/inbound/dom/sms/src/SmsManager.cpp:308

which is

   nsresult rv = smsDBService->CreateMessageList(filter, aReverse, request);
>  if (NS_FAILED(rv)) {

Forget about static checking, and leave aside log or error messages, it's incredibly sad that gdb can't determine enough information about the failure here to do anything useful.  Will need an extra hour of sleep tonight to make progress tomorrow, I think.

Comment 40

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

bent, I've had a few things come up unexpectedly in the last few days.  Do you think you can take a look at this?

Comment 41

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Gregor enthusiastically volunteered to look into this! :)

Comment 42

[Gregor Wagner [:gwagner]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #41)
> Gregor enthusiastically volunteered to look into this! :)


I will look at it tomorrow on the device when I have access to my desktop again. My laptop is too slow to compile for the device.

On my desktop build I see following when I kill the app but I think that's another bug:
Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: 13 at address: 0x0000000000000000
0x000000010329279f in mozilla::dom::ContentParent::DeallocPSms (this=0x11be8a000, aSms=0x11f6b1380) at /Users/idefix2/code/gaia/isrc/dom/ipc/ContentParent.cpp:1468
1468	    delete aSms;
(gdb) p aSms
$1 = (PSmsParent *) 0x11f6b1380
(gdb) p *aSms
$2 = {
  <mozilla::ipc::RPCChannel::RPCListener> = {
    <mozilla::ipc::SyncChannel::SyncListener> = {
      <mozilla::ipc::AsyncChannel::AsyncListener> = {
        <mozilla::ipc::HasResultCodes> = {<No data fields>}, 
        members of mozilla::ipc::AsyncChannel::AsyncListener: 
        _vptr$AsyncListener = 0x5a5a5a5a5a5a5a5a
      }, <No data fields>}, <No data fields>}, 
  <mozilla::ipc::IProtocolManager<mozilla::ipc::RPCChannel::RPCListener>> = {
    _vptr$IProtocolManager = 0x5a5a5a5a5a5a5a5a
  }, 
  members of mozilla::dom::sms::PSmsParent: 
  mChannel = 0x5a5a5a5a5a5a5a5a, 
  mManager = 0x5a5a5a5a5a5a5a5a, 
  mId = 1515870810, 
  mState = 1515870810
}

Comment 43

[Justin Lebar (not reading bugmail)]

> On my desktop build I see following when I kill the app but I think that's another bug:

In case it's not clear, this is almost surely a double-free (with 0x5a being what we're filling the freed space with).

Comment 44

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is almost certainly a different issue than we're seeing on device.

Comment 45

[Eric Shepherd [:sheppy]]

Marking as impacting docs since there are IDL changes.

Comment 46

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The changes here are all within Gecko, no public APIs will change.

Comment 47

[Gregor Wagner [:gwagner]]

Attachment: Part D: Really remove SmsRequestManager rebased

rebased

Comment 48

[Gregor Wagner [:gwagner]]

I think this overloading services doesn't work here.

In SmsManager::GetMessages we get the Service with SMS_DATABASE_SERVICE_CONTRACTID.

   nsCOMPtr<nsISmsDatabaseService> smsDBService =
    do_GetService(SMS_DATABASE_SERVICE_CONTRACTID);
  NS_ENSURE_TRUE(smsDBService, NS_ERROR_FAILURE);

  smsDBService->CreateMessageList(filter, aReverse, static_cast<SmsRequest*>(*aRequest));


We create the Service in the ServicesFactory with 
smsDBService = do_GetService(RIL_SMS_DATABASE_SERVICE_CONTRACTID);


So we use a different contractID (RIL_SMS_DATABASE_SERVICE...) and this is connected to SmsDatabaseService.js.

For the actual smsDBService->CreateMessageList call we use a different contractID(SMS_DATABASE_SERVICE_CONTRACTID)

I don't think this dispatch works during runtime. But maybe there are just some entries in the nsLayoutModule.cpp missing.

We need some contractID experts here.

Comment 49

[Gregor Wagner [:gwagner]]

Attachment: All in One (A-D)

Comment 50

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

mrbkap, any thoughts?

I don't like hearing phrases like "contract ID overloading", so my default reaction is to de-COM as much as possible.

Comment 51

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #50)
> mrbkap, any thoughts?

Here's what's happening. Fortunately the problem isn't related to contract ID's.

In the patch, we create an SmsRequest as:

  nsresult rv = SmsRequest::Create(this, getter_AddRefs(request));

And then we pass it to our (chrome only) interface in:

  rv = smsDBService->CreateMessageList(filter, aReverse,
                                       static_cast<SmsRequest*>(*aRequest));

However, SmsRequest doubles as the DOM SmsRequest as well, meaning that it has a scriptable helper in nsDOMClassInfo.cpp. All DOM objects using DOM_DEFAULT_SCRIPTABLE_FLAGS will refuse to be QI'd to anything not exposed in nsDOMClassInfo (which only exposes the DOM interfaces). Therefore, when we go to call into the JS implemented dbservice, XPConnect throws during argument conversion.

The easiest fix for this would be to create a small implementation of nsISmsRequest (not DOM) that forwards to the real implementation when it's called, I think. At least that way we'd avoid the odd "using the same object for DOM and chrome interfaces" problem.

Comment 52

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm trying to parse your comment, but in parallel, is it possible for us to get an error message or something when this happens?  The fact that it took at least a person-week of work from four people to track this down is pretty bad ;).

Comment 53

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

gwagner/mrbkap/bent, can one of you guys take this?

Comment 54

[Gregor Wagner [:gwagner]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #53)
> gwagner/mrbkap/bent, can one of you guys take this?

mrbkap is on vacation. I keep it.

Comment 55

[Gregor Wagner [:gwagner]]

Attachment: Part E: Fix QI Problems

big thx to bent!

This fixes the QI problem when we convert the argument.

Comment 56

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 670590 [details] [diff] [review]
Part E: Fix QI Problems

Review of attachment 670590 [details] [diff] [review]:
-----------------------------------------------------------------

A couple of drive-by comments. Thanks for picking this up!

::: dom/sms/src/SmsRequest.h
@@ +18,5 @@
>  namespace sms {
> +  
> +class SmsRequestForwarder : public nsISmsRequest
> +{
> +  NS_DECL_ISUPPORTS

This leaves the class in a public: section, but it'd be a ton cleaner to add an explicit public: above this.

@@ +26,5 @@
> +  nsCOMPtr<nsISmsRequest> mRealRequest;
> +  
> +  SmsRequestForwarder(nsISmsRequest* aRealRequest) {
> +    mRealRequest = aRealRequest;
> +  };

Nit: This semicolon and the one after the destructor are redundant.

@@ +29,5 @@
> +    mRealRequest = aRealRequest;
> +  };
> +  ~SmsRequestForwarder() {};
> +};
> + 

Here and above: trailing whitespace.

Comment 57

[Gregor Wagner [:gwagner]]

Attachment: New Patch

I can send, receive, delete SMS messages now OOP.
The message app still crashes when I close/kill it and there are still some random crashes.

Comment 58

[Gregor Wagner [:gwagner]]

Attachment: Patch

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=719186d95c08

Comment 59

[Gregor Wagner [:gwagner]]

Attachment: patch

We have to figure out what to do with the android implementation!

Comment 60

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hey Mounir,

Were you able to catch up with bent and gwagner about the android bits here?  I think that's the remaining blocking piece.

Comment 61

[Mounir Lamouri (:mounir)]

Greg said that he doesn't have time to update the Android bits so I asked Vicamo if he could do that because he did some great work in updating the Android backend for his latest WebSMS patches. According to him, there is an issue with nsISmsMessage that needs to be hold by the backend and could be hard to do from Java. I don't really know all the details though.

As much as I agree that we shouldn't hold this patch on the Android backend, I think it is important to evaluate the feasibility of the Android implementation and at least give it a try. I believe Vicamo would be the best person to work on that because he did some work on this patch and knows how the Android backend works.

Comment 62

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I think Vicamo is extremely overloaded right now.  What's Plan B if Vicamo can't take this on?

Comment 63

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

Attachment: Part 2: fix android : WIP

TODO:
1) clean Java
2) create a TArray<nsCOMPtr<nsISmsRequest> > to keep these requests.

Comment 64

[Mounir Lamouri (:mounir)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #63)
> 2) create a TArray<nsCOMPtr<nsISmsRequest> > to keep these requests.

Last time we talked on IRC, we were concerned about the feasibility of this. What's your opinion on this?

Comment 65

[Gregor Wagner [:gwagner]]

Attachment: Remove Android

I used ifdef to remove the android implementation. Just a backup if vicamo needs more time.

Comment 66

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

Attachment: Part 2: fix android : WIP2

TODO: build & test?

Comment 67

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

Attachment: Part 2: fix Android OOP

Comment 68

[:gerard-majax]

I tried applying those patches on current tree, and it failed for the last part:
alex@portable-alex:~/codaz/B2G/gecko$ patch -p1 < bug775997-part0.patch 
patching file dom/ipc/ContentParent.cpp
Hunk #2 succeeded at 1496 (offset 1 line).
patching file dom/sms/interfaces/Makefile.in
patching file dom/sms/interfaces/nsIDOMSmsCursor.idl
patching file dom/sms/interfaces/nsIDOMSmsManager.idl
patching file dom/sms/interfaces/nsISmsDatabaseService.idl
patching file dom/sms/interfaces/nsISmsRequest.idl
patching file dom/sms/interfaces/nsISmsRequestManager.idl
patching file dom/sms/interfaces/nsISmsService.idl
patching file dom/sms/src/Makefile.in
patching file dom/sms/src/SmsCursor.cpp
patching file dom/sms/src/SmsCursor.h
patching file dom/sms/src/SmsManager.cpp
patching file dom/sms/src/SmsRequest.cpp
patching file dom/sms/src/SmsRequest.h
patching file dom/sms/src/SmsRequestManager.cpp
patching file dom/sms/src/SmsRequestManager.h
patching file dom/sms/src/SmsServicesFactory.cpp
patching file dom/sms/src/android/SmsDatabaseService.cpp
patching file dom/sms/src/android/SmsService.cpp
patching file dom/sms/src/fallback/SmsDatabaseService.cpp
patching file dom/sms/src/fallback/SmsService.cpp
patching file dom/sms/src/ipc/PSms.ipdl
patching file dom/sms/src/ipc/PSmsRequest.ipdl
patching file dom/sms/src/ipc/SmsChild.cpp
patching file dom/sms/src/ipc/SmsChild.h
patching file dom/sms/src/ipc/SmsIPCService.cpp
patching file dom/sms/src/ipc/SmsIPCService.h
patching file dom/sms/src/ipc/SmsParent.cpp
patching file dom/sms/src/ipc/SmsParent.h
patching file dom/sms/src/ipc/SmsTypes.ipdlh
patching file dom/sms/src/ipc/ipdl.mk
patching file dom/sms/src/ril/SmsDatabaseService.js
patching file dom/sms/src/ril/SmsService.cpp
patching file dom/system/gonk/RadioInterfaceLayer.js
Hunk #2 succeeded at 1356 (offset 89 lines).
Hunk #3 succeeded at 1387 (offset 89 lines).
Hunk #4 succeeded at 1413 (offset 89 lines).
Hunk #5 succeeded at 2234 (offset 90 lines).
patching file dom/system/gonk/nsIRadioInterfaceLayer.idl
patching file layout/build/nsLayoutModule.cpp
alex@portable-alex:~/codaz/B2G/gecko$ patch -p1 < bug775997-part1.patch 
patching file dom/sms/src/android/SmsDatabaseService.cpp
patching file dom/sms/src/android/SmsService.cpp
patching file mozglue/android/APKOpen.cpp
patching file widget/android/AndroidBridge.cpp
patching file widget/android/AndroidBridge.h
patching file widget/android/AndroidJNI.cpp
alex@portable-alex:~/codaz/B2G/gecko$ patch -p1 < bug775997-part2.patch 
patching file dom/sms/src/android/SmsDatabaseService.cpp
Hunk #1 FAILED at 40.
1 out of 1 hunk FAILED -- saving rejects to file dom/sms/src/android/SmsDatabaseService.cpp.rej
patching file dom/sms/src/android/SmsService.cpp
Hunk #1 FAILED at 36.
1 out of 1 hunk FAILED -- saving rejects to file dom/sms/src/android/SmsService.cpp.rej
patching file embedding/android/GeckoAppShell.java
patching file embedding/android/GeckoSmsManager.java
patching file embedding/android/SmsManager.java.in
patching file mobile/android/base/GeckoAppShell.java
patching file mobile/android/base/GeckoSmsManager.java
patching file mobile/android/base/SmsManager.java.in
patching file mozglue/android/APKOpen.cpp
Hunk #1 FAILED at 303.
1 out of 1 hunk FAILED -- saving rejects to file mozglue/android/APKOpen.cpp.rej
patching file widget/android/AndroidBridge.cpp
Hunk #1 FAILED at 151.
Hunk #2 succeeded at 1713 (offset 4 lines).
Hunk #3 succeeded at 1749 (offset 4 lines).
Hunk #4 FAILED at 1795.
2 out of 4 hunks FAILED -- saving rejects to file widget/android/AndroidBridge.cpp.rej
patching file widget/android/AndroidBridge.h
Hunk #2 FAILED at 315.
Hunk #3 succeeded at 395 (offset 2 lines).
1 out of 3 hunks FAILED -- saving rejects to file widget/android/AndroidBridge.h.rej
patching file widget/android/AndroidJNI.cpp
Hunk #1 FAILED at 29.
Hunk #2 succeeded at 242 (offset 4 lines).
Hunk #3 FAILED at 323.
2 out of 3 hunks FAILED -- saving rejects to file widget/android/AndroidJNI.cpp.rej

Comment 69

[:gerard-majax]

Sorry for the previous one, applying part0 and part2 is okay.

Comment 70

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 674910 [details] [diff] [review]
patch

Review of attachment 674910 [details] [diff] [review]:
-----------------------------------------------------------------

Only 121k? I'm disappointed.

Only a few bigish things here, overall looks good!

::: dom/ipc/ContentParent.cpp
@@ +28,5 @@
>  #include "mozilla/dom/StorageParent.h"
>  #include "mozilla/dom/bluetooth/PBluetoothParent.h"
>  #include "mozilla/dom/devicestorage/DeviceStorageRequestParent.h"
> +#include "mozilla/dom/sms/PSmsParent.h"
> +#include "SmsParent.h"

Nit: SmsParent.h should include PSmsParent.h always, you could only include the latter.

::: dom/sms/interfaces/nsISmsService.idl
@@ +16,5 @@
>  interface nsISmsService : nsISupports
>  {
>    boolean        hasSupport();
>    unsigned short getNumberOfMessagesForText(in DOMString text);
> +            void send(in DOMString number,

Nit: Can you align this better?

::: dom/sms/src/SmsCursor.cpp
@@ +83,3 @@
>    mMessage = nullptr;
> +  request->Reset();
> +  mRequest->AddRef();

If the GetService below fails then you'll leak here, so I'd prefer something like this:

  nsRefPtr<SmsRequest> request = static_cast<SmsRequest*>(mRequest.get());
  // Things that can fail...
  request.forget();
  return NS_OK;

::: dom/sms/src/SmsManager.cpp
@@ +155,5 @@
>  
>    nsDependentJSString number;
>    number.init(aCx, aNumber);
> +  
> +  nsCOMPtr<nsISmsRequest> forwarder = 

Nit: whitespace on this line and the previous.

@@ +220,5 @@
>  
>  NS_IMETHODIMP
>  SmsManager::GetMessageMoz(int32_t aId, nsIDOMMozSmsRequest** aRequest)
>  {
> +  nsresult rv = SmsRequest::Create(this, aRequest);

Blegh, I really don't like assigning XPCOM things into outparams before we know we're going to return a success code. Can you rework these to have a local nsCOMPtr?

::: dom/sms/src/SmsRequest.cpp
@@ +63,5 @@
>  
>  NS_IMPL_EVENT_HANDLER(SmsRequest, success)
>  NS_IMPL_EVENT_HANDLER(SmsRequest, error)
>  
> +/* static */nsresult

Nit: space between /n

@@ +68,5 @@
> +SmsRequest::Create(SmsManager* aManager,
> +                   nsIDOMMozSmsRequest** aRequest)
> +{
> +  nsCOMPtr<nsIDOMMozSmsRequest> request = new SmsRequest(aManager);
> +  request->AddRef();

Hm, the more I see this the more I think this is wrong. We shouldn't double-addref here, we should make the service hold the request alive while it's in flight.

@@ +91,5 @@
>  {
>    BindToOwner(aManager);
>  }
>  
> +SmsRequest::SmsRequest(SmsRequestParent* requestParent)

I'd assert a non-null actor here, and prefix the param name with 'a'

@@ +312,5 @@
> +
> +nsresult
> +SmsRequest::SendMessageReply(const MessageReply& aReply)
> +{
> +  if(mParentAlive) {

Nit: Space between f(

@@ +385,5 @@
> +{
> +  if (mParent) {
> +    SmsMessageData data = SmsMessageData(static_cast<SmsMessage*>(aMessage)->GetData());
> +    return SendMessageReply(MessageReply(ReplyCreateMessageList(aListId, data)));
> +  } else {

Nit: else after return, here and a bunch of other places.

::: dom/sms/src/SmsRequest.h
@@ +18,5 @@
>  namespace sms {
> +
> +class SmsRequestParent;
> +class MessageReply;
> +  

Nit: Some whitespace problems in this file too.

@@ +25,5 @@
> +  NS_FORWARD_NSISMSREQUEST(mRealRequest->)
> +  
> +  NS_DECL_ISUPPORTS
> +
> +  nsCOMPtr<nsISmsRequest> mRealRequest;

This is public and it shouldn't be.

@@ +29,5 @@
> +  nsCOMPtr<nsISmsRequest> mRealRequest;
> +  
> +  SmsRequestForwarder(nsISmsRequest* aRealRequest) {
> +    mRealRequest = aRealRequest;
> +  };

Nit: Remove unneeded ;

@@ +31,5 @@
> +  SmsRequestForwarder(nsISmsRequest* aRealRequest) {
> +    mRealRequest = aRealRequest;
> +  };
> +  virtual
> +  ~SmsRequestForwarder() {};

Nit: Here too.

@@ +115,5 @@
> +  template <class T>
> +  nsresult NotifySuccess(T aParam);
> +  nsresult NotifyError(int32_t aError);
> +
> +  static nsTArray<nsCOMPtr<nsISmsRequest> > sRequests;

This isn't needed any more, right?

::: dom/sms/src/ipc/PSms.ipdl
@@ +72,3 @@
>  
> +  NotifySentMessage(SmsMessageData aMessageData);
> + 

Nit: extra whitespace here and another spot below.

@@ +99,2 @@
>  
> +  ClearMessageList(int32_t aListId);

Hm, why isn't this a request too?

::: dom/sms/src/ipc/SmsChild.cpp
@@ +19,5 @@
> +}
> +
> +SmsChild::~SmsChild()
> +{
> +  MOZ_COUNT_DTOR(SmsChild);

Where's the matching _CTOR call?

@@ +86,5 @@
>  
> +/*******************************************************************************
> + * SmsRequestChild
> + ******************************************************************************/
> +;

Eh?

@@ +157,2 @@
>    }
> +  

Nit: Whitespace problem here.

::: dom/sms/src/ipc/SmsChild.h
@@ +16,5 @@
>  namespace sms {
>  
>  class SmsChild : public PSmsChild
>  {
> +

Nit: Nuke extra newline

@@ +28,5 @@
> +  ActorDestroy(ActorDestroyReason aWhy) MOZ_OVERRIDE;
> +
> +  virtual bool
> +  RecvNotifyReceivedMessage(const SmsMessageData& aMessage) MOZ_OVERRIDE;
> +  

Nit: Whitespace problems in this file.

@@ +47,5 @@
> +{
> +  friend class mozilla::dom::sms::SmsChild;
> +
> +  nsCOMPtr<nsISmsRequest> mReplyRequest;
> +public:

Nit: Need a newline between those two lines.

::: dom/sms/src/ipc/SmsIPCService.cpp
@@ +15,5 @@
>  namespace mozilla {
>  namespace dom {
>  namespace sms {
>  
> +PSmsChild* sSmsChild;

Since this is a global now (not a static member) use 'gSmsChild'

::: dom/sms/src/ipc/SmsParent.cpp
@@ +35,5 @@
>  }
>  
> +SmsParent::~SmsParent()
> +{
> +  MOZ_COUNT_DTOR(SmsParent);

Yay, but where is the matching MOZ_COUNT_CTOR?

@@ +203,5 @@
> + * SmsRequestParent
> + ******************************************************************************/
> +SmsRequestParent::SmsRequestParent()
> +{
> + MOZ_COUNT_CTOR(SmsRequestParent);

Nit: This and the DTOR need two-space indent, not one :)

@@ +234,5 @@
> +SmsRequestParent::DoRequest(const SendMessageRequest& aRequest)
> +{
> +  nsCOMPtr<nsISmsService> smsService = do_GetService(SMS_SERVICE_CONTRACTID);
> +  NS_ENSURE_TRUE(smsService, true);
> +  

Nit: Whitespace here and some others in this file

@@ +237,5 @@
> +  NS_ENSURE_TRUE(smsService, true);
> +  
> +  mSmsRequest = SmsRequest::Create(this);
> +  nsCOMPtr<nsISmsRequest> forwarder = new SmsRequestForwarder(mSmsRequest);
> +  smsService->Send(aRequest.number(), aRequest.message(), forwarder);

Hm, this can fail right? What should happen if it does? Same problem in all the following methods.

::: dom/sms/src/ipc/SmsParent.h
@@ +32,5 @@
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIOBSERVER
>  
> +  virtual bool
> +  RecvHasSupport(bool* aHasSupport) MOZ_OVERRIDE;

All these should be protected, I think. They should only be called by IPDL.

@@ +64,5 @@
> +  virtual bool
> +  DeallocPSmsRequest(PSmsRequestParent* aActor) MOZ_OVERRIDE;
> +};
> +
> +class SmsRequest;

Nit: Let's move this to the top of the file.

@@ +74,5 @@
> +
> +  nsRefPtr<SmsRequest> mSmsRequest;
> +
> +public:
> +

Nit: Nuke this extra line.

::: dom/sms/src/ipc/SmsTypes.ipdlh
@@ +83,5 @@
> +struct ReplyNoMessageInList
> +{
> +};
> +
> +union MessageReply

I think all these reply types and this union can just live in PSmsRequest.ipdl, right?

Comment 71

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 675618 [details] [diff] [review]
Part 2: fix Android OOP

Review of attachment 675618 [details] [diff] [review]:
-----------------------------------------------------------------

I'd just be rubber stamping this patch

Comment 72

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 73

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

(In reply to Gregor Wagner [:gwagner] from comment #72)
> Created attachment 676790 [details] [diff] [review]
> patch

Gregor, you don't have to worry about rebasing your patch to mine. I've been working on this since last night and have already some results in my local branch[1]. But somehow emulator is broken yet again, b2g keeps crashing, so I have to spend extra time on it. Anyway, I'll rebase your last patch and upload one for you.

[1]: https://github.com/vicamo/mozilla-central/tree/bugzilla/775997/rebase-742790

Comment 74

[Doug Turner (:dougt)]

Comment on attachment 675618 [details] [diff] [review]
Part 2: fix Android OOP

Review of attachment 675618 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Vicamo,  mostly looks fine.  I'd like to see another patch.  Maybe when bent's done with his review - in case there is fallout?

::: widget/android/AndroidBridge.cpp
@@ +1830,5 @@
>  }
>  
> +int32_t
> +AndroidBridge::QueueSmsRequest(nsISmsRequest* aRequest)
> +{

Please add:
  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

@@ +1831,5 @@
>  
> +int32_t
> +AndroidBridge::QueueSmsRequest(nsISmsRequest* aRequest)
> +{
> +    for (int32_t i = 0; i < mSmsRequests.Length(); i++) {

probably pulling .Length() out of this loop is a good idea.

@@ +1842,5 @@
> +    return mSmsRequests.Length() - 1;
> +}
> +
> +bool
> +AndroidBridge::DequeueSmsRequest(int32_t aRequestId, nsISmsRequest** aRequest)

Instead of an inout parameter, the code would be a bit cleaner if you did:

already_AddRefed<nsISmsRequest>
AndroidBridge::DequeueSmsRequest(int32_t aRequestId)

@@ +1843,5 @@
> +}
> +
> +bool
> +AndroidBridge::DequeueSmsRequest(int32_t aRequestId, nsISmsRequest** aRequest)
> +{

Same, add:
  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

These methods must never run on any other thread.

@@ +1850,5 @@
> +    }
> +
> +    nsCOMPtr<nsISmsRequest> req = mSmsRequests[aRequestId];
> +    mSmsRequests[aRequestId] = nullptr;
> +    req.forget(aRequest);

What is the point of keeping null's in the array?  it is to avoid the array from being compacted?

::: widget/android/AndroidBridge.h
@@ +399,5 @@
>      bool mHasNativeWindowFallback;
>  
>      int mAPIVersion;
>  
> +    nsTArray<nsCOMPtr<nsISmsRequest> > mSmsRequests;

I worry that this might lead to shutdown leaks.  I'd feel better if you cleared this list at application shutdown.  Note that most of the AndroidBridge does not hold onto xpcom objects.

::: widget/android/AndroidJNI.cpp
@@ +620,5 @@
>  
>      nsJNIString receiver = nsJNIString(aReceiver, jenv);
>      DeliveryState state = receiver.IsEmpty() ? eDeliveryState_Received
>                                               : eDeliveryState_Sent;
>   

while you're here, trailing whitespace (M-x delete-trailing-whitespace)

Comment 75

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #73)
> But somehow emulator is broken yet again, b2g keeps crashing, so
> I have to spend extra time on it.

Rebuild whole system image fixes above problems, but bug 798491 hangs B2G Marionette forever.

Comment 76

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

Attachment: Part 1: DOM

Rebase Gregor's patch to inbound tip(to include changes made in bug 742790, bug 797277)

Comment 77

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

Attachment: Part 2: Android implementation

Rebase Android part to inbound tip(to include changes made in bug 742790, bug 797277). Not request for review for this patch has not address any review comment in comment #74.

Comment 78

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

(In reply to Doug Turner (:dougt) from comment #74)
> Review of attachment 675618 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hey Vicamo,  mostly looks fine.  I'd like to see another patch.  Maybe when
> bent's done with his review - in case there is fallout?

Sure, but I'll still upload refreshed patch ASAP.

> ::: widget/android/AndroidBridge.cpp
> @@ +1850,5 @@
> > +    }
> > +
> > +    nsCOMPtr<nsISmsRequest> req = mSmsRequests[aRequestId];
> > +    mSmsRequests[aRequestId] = nullptr;
> > +    req.forget(aRequest);
> 
> What is the point of keeping null's in the array?  it is to avoid the array
> from being compacted?

We have a XPCOM <-> index transition here. The index shall not be reused and the kept nsCOMPtr should not be moved to a different place. Removing an element from that array invalidates such mapping.

> ::: widget/android/AndroidBridge.h
> @@ +399,5 @@
> >      bool mHasNativeWindowFallback;
> >  
> >      int mAPIVersion;
> >  
> > +    nsTArray<nsCOMPtr<nsISmsRequest> > mSmsRequests;
> 
> I worry that this might lead to shutdown leaks.  I'd feel better if you
> cleared this list at application shutdown.  Note that most of the
> AndroidBridge does not hold onto xpcom objects.

In B2G RIL backend, the possession is automatically done by XPConnect. Holding a reference to the request object must happen in every backend that implements WebSMS.

Comment 79

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

Attachment: Part 2: Android implementation : v2

Addresses review comment #74 but cleanup at shutting down.

AndroidBridge doesn't inherit nsISupports or other XPCOM interfaces. To listen "xpcom-shutdown" event, I'll have to make it inherits nsIObserver. Is this something I should do?

Comment 80

[Gregor Wagner [:gwagner]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #73)
> (In reply to Gregor Wagner [:gwagner] from comment #72)
> > Created attachment 676790 [details] [diff] [review]
> > patch
> 
> Gregor, you don't have to worry about rebasing your patch to mine. I've been
> working on this since last night and have already some results in my local
> branch[1]. But somehow emulator is broken yet again, b2g keeps crashing, so
> I have to spend extra time on it. Anyway, I'll rebase your last patch and
> upload one for you.
> 

Thanks Vicamo for merging! Next time please let me know before you land a patch that collides so that we can coordinate better :)

Comment 81

[Gregor Wagner [:gwagner]]

Hm doesn't seem to work. Investigating...

E/GeckoConsole( 2176): [JavaScript Error: "[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsISmsService.createSmsMessage]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: jar:file:///system/b2g/omni.ja!/components/SmsDatabaseService.js :: oncomplete :: line 327"  data: no]" {file: "jar:file:///system/b2g/omni.ja!/components/SmsDatabaseService.js" line: 327}]

Comment 82

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

(In reply to Gregor Wagner [:gwagner] from comment #81)
> Hm doesn't seem to work. Investigating...
> 
> E/GeckoConsole( 2176): [JavaScript Error: "[Exception... "Component returned
> failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE)
> [nsISmsService.createSmsMessage]"  nsresult: "0x80070057
> (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame ::
> jar:file:///system/b2g/omni.ja!/components/SmsDatabaseService.js ::
> oncomplete :: line 327"  data: no]" {file:
> "jar:file:///system/b2g/omni.ja!/components/SmsDatabaseService.js" line:
> 327}]

Thank you for opening and solving bug 807463 for this!

Comment 83

[Gregor Wagner [:gwagner]]

Hm it seems now I can't send in-process SMS any more with the merged patch.

Comment 84

[Gregor Wagner [:gwagner]]

After more testing:
- receiving doesn't work OOP
- sending doesn't work in process

Comment 85

[Gregor Wagner [:gwagner]]

(In reply to Gregor Wagner [:gwagner] from comment #84)
> After more testing:
> - receiving doesn't work OOP
> - sending doesn't work in process

Seems like I hit bug 807631.

Comment 86

[Gregor Wagner [:gwagner]]

Attachment: patch

Change Ci.nsIDOMSmsRequest to Ci.nsISmsRequest in RadioInterfaceLayer.js

Comment 87

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This patch works great for me.  \o/  I didn't see a regression in startup either, but that's not saying much ;).  (It slow.)

I knocked the SMS service OOP, but the difference in memory usage was inconclusive in the main process.  We'll be removing it anyway, so it doesn't matter too much.

As expected, this incidentally fixes the crappy keyboard perf in the SMS app.

Comment 88

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 677931 [details] [diff] [review]
patch

Review of attachment 677931 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these nits fixed and the followups (noted below) filed. Looks great!

::: dom/sms/src/SmsCursor.cpp
@@ +88,5 @@
>    NS_ENSURE_TRUE(smsDBService, NS_ERROR_FAILURE);
>  
> +  nsCOMPtr<nsISmsRequest> forwarder = new SmsRequestForwarder(request);
> +  smsDBService->GetNextMessageInList(mListId, forwarder);
> +  request.forget();

Since it's not really clear and easy to miss I think we should add a big comment here saying why we're intentionally adding a ref here and where the corresponding release happens.

::: dom/sms/src/SmsManager.cpp
@@ +162,5 @@
>  
> +  nsCOMPtr<nsISmsRequest> forwarder =
> +    new SmsRequestForwarder(static_cast<SmsRequest*>(request.get()));
> +
> +  smsService->Send(number, aMessage, forwarder);

Hm, we're not handling failures here or anywhere else where we call into the smsService. These methods aren't infallible, right?

I realize the old code did this too so I'd be fine doing this in a followup if that makes life easier.

::: dom/sms/src/SmsRequest.cpp
@@ +418,5 @@
> +SmsRequest::NotifyNoMessageInList()
> +{
> +  if (mParent) {
> +    return SendMessageReply(MessageReply(ReplyNoMessageInList()));
> +  } else {

Nit: else-after-return

::: dom/sms/src/SmsRequest.h
@@ +19,5 @@
> +
> +class SmsRequestParent;
> +class MessageReply;
> +
> +class SmsRequestForwarder : public nsISmsRequest

This needs a comment saying why it exists (to avoid a QI to nsIClassInfo, etc.)

@@ +55,5 @@
>    NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(SmsRequest,
>                                                           nsDOMEventTargetHelper)
>  
> +  static nsresult Create(SmsManager* aManager,
> +                         nsIDOMMozSmsRequest** aRequest);

We should switch this to return 'already_AddRefed<SmsRequest>' also since we're adding it in this patch.

::: dom/sms/src/ipc/PSmsRequest.ipdl
@@ +31,5 @@
> +};
> +
> +struct ReplyMessageSendFail
> +{
> +  int32_t error;

Are these actually nsresults? If so you can just use nsresult in the IPDL.

::: dom/sms/src/ipc/SmsChild.cpp
@@ +118,4 @@
>  {
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(mReplyRequest);
> +  nsCOMPtr<SmsMessage> message;

It doesn't look like this is used, but rather then remove it I would prefer us to use it. In each of the cases below that make a new SmsMessage we should first assign that to the nsCOMPtr, and then pass that to the Notify() function.

::: dom/sms/src/ipc/SmsParent.cpp
@@ +260,5 @@
> +bool
> +SmsRequestParent::DoRequest(const SendMessageRequest& aRequest)
> +{
> +  nsCOMPtr<nsISmsService> smsService = do_GetService(SMS_SERVICE_CONTRACTID);
> +  NS_ENSURE_TRUE(smsService, true);

Returning true here will basically ignore this request, and the child process will never see a response.

Below, returning false if Send() fails will kill the child.

I'd argue that both of these are not the desired outcome. Instead I think we should be sending error replies when things fail. It's fine to fix this in a followup though.

Oh, and we have this same problem in most of the other DoRequest functions.

@@ +273,5 @@
>  
>  bool
> +SmsRequestParent::DoRequest(const GetMessageRequest& aRequest)
> +{
> +  nsCOMPtr<nsISmsDatabaseService> smsDBService = 

Nit: trailing whitespace.

::: dom/sms/src/ipc/SmsParent.h
@@ +7,5 @@
>  #define mozilla_dom_sms_SmsParent_h
>  
>  #include "mozilla/dom/sms/PSmsParent.h"
> +#include "mozilla/dom/sms/PSmsRequestParent.h"
> +#include "mozilla/dom/sms/SmsServicesFactory.h"

It doesn't look like this one is necessary... Is it?

Comment 89

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

(In reply to ben turner [:bent] from comment #88)
> Comment on attachment 677931 [details] [diff] [review]
> -----------------------------------------------------------------
> ::: dom/sms/src/SmsManager.cpp
> @@ +162,5 @@
> >  
> > +  nsCOMPtr<nsISmsRequest> forwarder =
> > +    new SmsRequestForwarder(static_cast<SmsRequest*>(request.get()));
> > +
> > +  smsService->Send(number, aMessage, forwarder);
> 
> Hm, we're not handling failures here or anywhere else where we call into the
> smsService. These methods aren't infallible, right?
> 
> I realize the old code did this too so I'd be fine doing this in a followup
> if that makes life easier.

This interface function has void return type and should be infallible because all the errors should be fired in the backend handler function asynchronously.

> ::: dom/sms/src/ipc/PSmsRequest.ipdl
> @@ +31,5 @@
> > +};
> > +
> > +struct ReplyMessageSendFail
> > +{
> > +  int32_t error;
> 
> Are these actually nsresults? If so you can just use nsresult in the IPDL.

This attribute is for nsISmsRequest.*_ERROR codes, not nsresults.

> ::: dom/sms/src/ipc/SmsParent.cpp
> @@ +260,5 @@
> > +bool
> > +SmsRequestParent::DoRequest(const SendMessageRequest& aRequest)
> > +{
> > +  nsCOMPtr<nsISmsService> smsService = do_GetService(SMS_SERVICE_CONTRACTID);
> > +  NS_ENSURE_TRUE(smsService, true);
> 
> Returning true here will basically ignore this request, and the child
> process will never see a response.
> 
> Below, returning false if Send() fails will kill the child.
> 
> I'd argue that both of these are not the desired outcome. Instead I think we
> should be sending error replies when things fail. It's fine to fix this in a
> followup though.
> 
> Oh, and we have this same problem in most of the other DoRequest functions.

We have defined a fallback backend, so the do_GetService should be infallible. How about s/NS_ENSURE_TRUE/NS_ASSERTION/ ?

Comment 90

[Gregor Wagner [:gwagner]]

Attachment: patch

Review comments.

Comment 91

[Gregor Wagner [:gwagner]]

Vicamo, you only requested feedback for your patch. Is it ready for review?

Comment 92

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #89)
> This interface function has void return type and should be infallible
> because all the errors should be fired in the backend handler function
> asynchronously.

Ok. I would still say you should assert that they succeed rather than not checking the return at all.

> This attribute is for nsISmsRequest.*_ERROR codes, not nsresults.

Ah, right.

> We have defined a fallback backend, so the do_GetService should be
> infallible. How about s/NS_ENSURE_TRUE/NS_ASSERTION/ ?

I'd be ok with that. In general let's make sure we're always checking our return values.

Comment 93

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

(In reply to Gregor Wagner [:gwagner] from comment #91)
> Vicamo, you only requested feedback for your patch. Is it ready for review?

I had a problem in comment #79:

  AndroidBridge doesn't inherit nsISupports or other XPCOM interfaces. To listen
  "xpcom-shutdown" event, I'll have to make it inherits nsIObserver. Is this
  something I should do?

Comment 94

[Doug Turner (:dougt)]

Can you use:
  http://mxr.mozilla.org/mozilla-central/source/xpcom/base/ClearOnShutdown.h

Comment 95

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

Attachment: Part 2: Android implementation : v3

Make use of ClearOnShutdown() as suggested in comment #94. Thanks Doug.

Comment 96

[Doug Turner (:dougt)]

Comment on attachment 678853 [details] [diff] [review]
Part 2: Android implementation : v3

Review of attachment 678853 [details] [diff] [review]:
-----------------------------------------------------------------

javascript:void(0);

::: widget/android/AndroidBridge.cpp
@@ +60,5 @@
>  
>  // This isn't in AndroidBridge.h because including StrongPointer.h there is gross
>  static android::sp<AndroidRefable> (*android_SurfaceTexture_getNativeWindow)(JNIEnv* env, jobject surfaceTexture) = nullptr;
>  
> +static StaticAutoPtr<nsTArray<nsCOMPtr<nsISmsRequest> > > sSmsRequests;

nit: Please make this AndroidBridge::sSmsRequest

Comment 97

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e06de89b4511
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5f987ca72e

Comment 98

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

\o/

Comment 99

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

Follow-up for review comment #96
https://hg.mozilla.org/integration/mozilla-inbound/rev/72d12424677e

Comment 100

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/e06de89b4511
https://hg.mozilla.org/mozilla-central/rev/6a5f987ca72e
https://hg.mozilla.org/mozilla-central/rev/72d12424677e

Comment 101

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/46b35c333abe
https://hg.mozilla.org/releases/mozilla-aurora/rev/464c6f5186bb
https://hg.mozilla.org/releases/mozilla-aurora/rev/117940683b07

Comment 102

[Gian-Carlo Pascutto [:gcp]]

The Android part of this causes an Assertion failure that causes my debug builds to instantly crash on startup.

I think it's handling nsCOMPtr's off the main thread.

Can we back this out or fix ASAP?

Comment 103

[Gian-Carlo Pascutto [:gcp]]

Yeah, I think the problem is that AndroidBridge::ConstructBridge runs in the Java thread, not the main Gecko thread. The coded added is fiddling with (freeing) XPCOM pointers there, which is not good.

Comment 104

[Gregor Wagner [:gwagner]]

We should remove the 2 lines from AndroidBridge::ConstructBridge and provide a real fix later today.
Please try not to back out all 3 patches because many B2G patches are blocked by this.

Comment 105

[Gian-Carlo Pascutto [:gcp]]

Attachment: Fix the Android startup crash by removing the crashing code.

Reopening the bug as we need a real fix too.

Comment 106

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

Comment on attachment 679214 [details] [diff] [review]
Fix the Android startup crash by removing the crashing code.

Review of attachment 679214 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, but I will upload a patch to wrap these lines with MOZ_WEBSMS_BACKEND later.

Comment 107

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #105)
> Reopening the bug as we need a real fix too.

Let's please do this in a followup. One "fix" per bug is much easier to track.

Comment 108

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

Attachment: Fix the Android startup crash by removing the crashing code.

Comment 109

[Gian-Carlo Pascutto [:gcp]]

What's the point of wrapping these lines? If they're only used on Android, might as well remove them, as the crash will simply reappear if it ever gets defined (and enabling the WEBSMS thing sure seems to be the intention). If it's also used on B2G, I sure hope it has a different threading model or it will also crash every debug build.

I don't see any point in keeping code around that can never work.

Comment 110

[Doug Turner (:dougt)]

we should remove websms on android.  there are no tests, it isn't being used anywhere, there isn't any product plans to use it.  Dead code, difficult to maintain.  Most of the webapi team agrees.

i'll rubber stamp the patch.

Comment 111

[Alex Keybl [:akeybl]]

We're marking this bug with the C1 milestone since it follows the criteria of "remaining LOE:L work" (see https://etherpad.mozilla.org/b2g-convergence-schedule).

If this work is not finished by Nov19, this bug will need an exception and will be called out at the upcoming Exec Review.

Comment 112

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

(In reply to Gian-Carlo Pascutto (:gcp) from comment #109)
> I don't see any point in keeping code around that can never work.

DOM team is going to create a bug for removing Android WebSMS. Before that, I just want to keep it synced with B2G as much as I can. If someone is going to maintain this backend, I hope he get as much holes filled as possible.

Comment 113

[Doug Turner (:dougt)]

Comment on attachment 679257 [details] [diff] [review]
Fix the Android startup crash by removing the crashing code.

Review of attachment 679257 [details] [diff] [review]:
-----------------------------------------------------------------

This patch makes you leak, right?

Comment 114

[Doug Turner (:dougt)]

Comment on attachment 679214 [details] [diff] [review]
Fix the Android startup crash by removing the crashing code.

Review of attachment 679214 [details] [diff] [review]:
-----------------------------------------------------------------

r+ w/ a comment.

::: widget/android/AndroidBridge.cpp
@@ -82,5 @@
>      }
>      sBridge = bridge;
> -
> -    sSmsRequests = new nsTArray<nsCOMPtr<nsISmsRequest> >();
> -    ClearOnShutdown(&sSmsRequests);

This effectively disables queuing of sms requests on Android.  Can you add a comment that states something like that?

Comment 115

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 679214 [details] [diff] [review]
Fix the Android startup crash by removing the crashing code.

[Triage Comment] please land on aurora to fix the android bustage

Comment 116

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/mozilla-central/rev/1437578c371f
https://hg.mozilla.org/releases/mozilla-aurora/rev/4880b39fdcd9

Will make a new bug for deciding what to do with this on Android.