962988 - MobileMessage: B2G desktop OOP fails to open sms app

Status: RESOLVED WONTFIX

(Core :: DOM: Device Interfaces, defect)

Description

[Gregor Wagner [:gwagner]]

When turning OOP on for a debug b2g-desktop the sms app doesn't launch.

[Parent 65174] WARNING: NS_ENSURE_TRUE(mContinueCallback) failed: file /Users/Gregor/code/src/dom/mobilemessage/src/ipc/SmsParent.cpp, line 813
IPDL protocol error: Handler for PMobileMessageCursor returned error code

###!!! [Parent][DispatchAsyncMessage] Error: Processing error: message was deserialized, but the handler returned false (indicating failure)

Process 65174 stopped and restarted: thread 1 received signal: SIGCHLD

###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: hide.sms.pref.behind.ril.ifdef.patch

It seems like we return false to one of the IPC call. Which makes sense since the sms API needs RIL. So this patch just turn the API on behind a ifdef.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Before asking r? I actually need to test my theory :)

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Since we're trying to turn on OOP now, we should get this fixed soon as we're running into this problem again.

Comment 4

[Zac C (:zac)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #1)
> Created attachment 8369422 [details] [diff] [review]
> hide.sms.pref.behind.ril.ifdef.patch
> 
> It seems like we return false to one of the IPC call. Which makes sense
> since the sms API needs RIL. So this patch just turn the API on behind a
> ifdef.

Would this still mean Messages app can run on TBPL/Travis? Just that there's a lot of useful coverage there that I'm sure Julien would like to keep.

Comment 5

[Gregor Wagner [:gwagner]]

Kyle, can you test this patch?

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Well, in terms of messages app no longer crashing, yes, this seems to do the job. Unfortunately due to bug 993171 I can't really run it through integration tests to see if it fixes those.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8369422 [details] [diff] [review]
hide.sms.pref.behind.ril.ifdef.patch

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

Ok, I'm gonna go ahead and say no on this. Even if RIL isn't available, we shouldn't crash, and this patch all the functionality of the app, meaning it also breaks all the integration tests. We should be able to at least compose a message, even if we can't send it.

I've done some poking around at what's causing it to crash in OOP, and it looks like we're being overly aggressive in our error handling on the parent side, making assumptions about the lifetime of the child that are incorrect, breaking the synchronicity of the ipdl protocol and causing the crash. There should be a better way to do this.

Comment 9

[Gregor Wagner [:gwagner]]

This blocks us from enabling oop desktop tests. Vicamo, do you have cycles for this?

Comment 10

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

Hi, we should not have mozMobileMessage on b2g-desktop at the first place per bug 916605 -- functions unsupported on a certain platforms should not be exposed by default.  However, that's blocked by bug 859764.  As a result, one should never ever need to turn on SMS OOP tests on b2g-desktop and sms app will never work on b2g-desktop unless it gets a backend.  It seems Lissy have an idea about this in bug 989926.

Comment 11

[Gregor Wagner [:gwagner]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #10)
> Hi, we should not have mozMobileMessage on b2g-desktop at the first place
> per bug 916605 -- functions unsupported on a certain platforms should not be
> exposed by default.  However, that's blocked by bug 859764.  As a result,
> one should never ever need to turn on SMS OOP tests on b2g-desktop and sms
> app will never work on b2g-desktop unless it gets a backend.  It seems Lissy
> have an idea about this in bug 989926.

I agree that we shouldn't rely on a SMS backend on desktop. I care about a consistent behavior for in and out of process. We shouldn't be able to run the app in one scenario and kill it in the other scenario.
Can we get a fix so we can show an empty SMS app for the OOP case as well?

Comment 12

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

(In reply to Gregor Wagner [:gwagner] from comment #11)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #10)
> > Hi, we should not have mozMobileMessage on b2g-desktop at the first place
> > per bug 916605 -- functions unsupported on a certain platforms should not be
> > exposed by default.  However, that's blocked by bug 859764.  As a result,
> > one should never ever need to turn on SMS OOP tests on b2g-desktop and sms
> > app will never work on b2g-desktop unless it gets a backend.  It seems Lissy
> > have an idea about this in bug 989926.
> 
> I agree that we shouldn't rely on a SMS backend on desktop. I care about a
> consistent behavior for in and out of process. We shouldn't be able to run
> the app in one scenario and kill it in the other scenario.
> Can we get a fix so we can show an empty SMS app for the OOP case as well?

Yes, so I've proposed a patch in bug 859764 to hide mozMobileMessage on unsupported platforms.  It works in a similar way with patch proposed in comment 7.  We're also working on bug 958782, move WebSMS to WebIDL, so that we can hide interface names as well as navigation attribute node.

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

As far as I'm aware, B2G desktop is dead in favor of Mulet. Closing this, reopen if I'm wrong.