1184865 - [Messages][NG] Replace some methods in MessageManager with messaging service in conversation view

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Steve Chung [:steveck]]

Since message service and message API shim is ready, we can do some experiment to figure out how to make it work in current code. We might need to introduce some utils method for iframe creation and connection setup between service/shim context.

Comment 1

[Steve Chung [:steveck]]

WIP: https://github.com/steveck-chung/gaia/tree/new-message-messaging-service

First step is to use BridgeServiceMixin in MozMobileMessageShim

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master

Comment 3

[Steve Chung [:steveck]]

Comment on attachment 8638545 [details] [review]
[gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master

Hi,

It's still a WIP about migrate retrieveMMS to messaging service/shim(because settings service/shim are not ready yet and message sending need it). The concept of the shim and iframe was aligned with Oleg's in conversation service patch. MMS retrieval could work under this architecture, but with some unneglectable flaws:
- retrieveMMS mozMobileMessage API was executed and retrieve mms correctly, but the bridge will return reject with no response error.
- It only works for the first time. when I tried to retrieve another mms, message service method was no executed.

I'll spend more time to dig out the problem, and it would be great if you could find out any problem or bug in the patch. ni? Wilson since he might know the root cause of these issues.

Hi Wilson, do you have any idea that might related to these issues? The call path from view to API would be:

messaging-client(window context) -> messageing-service(sharedworker context) -> message-shim(iframe context)

And inside the message-shim, we'll return the result of retrieveMMS API since it should be the then-able DOMrequest.

Comment 4

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8638545 [details] [review]
[gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master

Looks good to me! Left some comments at GitHub.

I've tried to reproduce "no-response" in Fx Desktop, I noticed it once, enabled debugging in bridge, and could not able to reproduce after that, I'll try later once again, if you don't figure out the reason earlier :)

Thanks!

Comment 5

[Wilson Page [:wilsonpage]]

(In reply to Steve Chung [:steveck] from comment #3)
> Hi Wilson, do you have any idea that might related to these issues? The call
> path from view to API would be:

Are you using BroadcastChannel for the connection you're having issues with? If so I have recently landed a patch that might improve the connection handshake in some corner-cases.

Try running with the latest threads.js master build and let me know if this improves things. If not can you give me a reduced test-case or some STRs with 'expected' and 'actual' outcomes, cheese :)

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 8638545 [details] [review]
[gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master

left some feedback but overall this looks good

Comment 7

[Steve Chung [:steveck]]

Updated with the latest threads, but the 2 main issues mentioned above still existed :/

Sorry I didn't have the reduced test-case yet, but the STRs would be:
1) Enter the message settings panel from settings or message app, and turn off the MMS auto retrieve
2) Send the MMS from other devices to target device, and you'll receive a message with download button. Click download.

Expected: Original message removed and mms message downloaded later

Actual: Original message will turn into the error status briefly since it will return no response error first, but the retrieval is success actually and it'll be replaced new message later. And after this action completed, if you repeat the same steps again, the service will not work with this error:

I/Gecko   (  208): IPDL protocol error: Handler for PostMessages returned error code
I/Gecko   (  208): ###!!! [Parent][DispatchAsyncMessage] Error: (msgtype=0x880001,name=PMessagePort::Msg_PostMessages) Processing error: message was deserialized, but the handler returned false (indicating failure)

Please let me know if there is any other way that could provide more debugging information for you, thanks!

Comment 8

[Wilson Page [:wilsonpage]]

(In reply to Steve Chung [:steveck] from comment #7)
> Sorry I didn't have the reduced test-case yet, but the STRs would be:
> 1) Enter the message settings panel from settings or message app, and turn
> off the MMS auto retrieve
> 2) Send the MMS from other devices to target device, and you'll receive a
> message with download button. Click download.

Is there any way to mock this without actually having to send real MMS?

Comment 9

[Oleg Zasypkin [:azasypkin]]

Hey Wilson,

I'm not 100% sure, but you can try to reproduce this in Fx Desktop:

1. Checkout PR and run "DEBUG=1 make";
2. Open this profile in Nightly (/path_to_nightly/firefox -profile /path_to_gaia/profile-debug/);
3. Navigate to "app://sms.gaiamobile.org";
4. Enter conversation from "Tom O'Hare" and find message with "Donwload" link;
5. When you tap on that link we retrieve MMS using mozMobileMessage mock, but "bridge" machinery is the same as on device.

So you can try several times to go back to Inbox, then enter the "Tom O'Hare" conversation and tap on Download link again - I noticed "no response" after few tries, but didn't have time to confirm that it's the same issue Steve is suffering from.

Comment 10

[Steve Chung [:steveck]]

I updated the patch to make the desktop mock runnable, and I found one issue is we'll need to make a clone for the returning message from API, otherwise service side could not be resolved successfully. Wondering why there is no dataClone error for this... Anyway if return the cloned message from shim, the no response error will be resolved. Still trying to figure out why the second service call could not work.

Comment 11

[Steve Chung [:steveck]]

Ok I think the latest commit addressed/workaround the dataclone error and sharedworker reference problem. One last question is about the no response timeout, since mms retrieval will need more time than default 1 sec timeout, is there an alternative way to change the timeout duration for some specific case?

Comment 12

[Steve Chung [:steveck]]

Comment on attachment 8638545 [details] [review]
[gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master

Hey,

Patch with the updated unit test is ready(hopefully), There's also update for bridge lib included for some changes for timeout feature and endpoint changes(that we can provide the sharedworker directly as endpoint). Please ping me if it didn't work on your device/desktop or seeing any performance regression, thanks!

Comment 13

[Steve Chung [:steveck]]

Clear ni? since Wilson already addressed the timeout issue in the latest master.

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 8638545 [details] [review]
[gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master

I left some comments on github.

This will need rebasing after bug 1162030 but the rebase is not very difficult (I managed to do it :) ).

I tested on the phone a MMS retrieval and it works fine, except we have the timeout.

Comment 15

[Julien Wajsberg [:julienw]]

I realize I also got this message once:

08-10 18:49:32.677  6817  6817 E Messages: [JavaScript Error: "DataCloneError: The object could not be cloned." {file: "app://sms.gaiamobile.org/lib/bridge/bridge.js" line: 17}]


but I don't know when it happened exactly... :/

Comment 16

[Julien Wajsberg [:julienw]]

Depends on https://github.com/gaia-components/threads/issues/73 (which is p=1 for this sprint as well)

Comment 17

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8638545 [details] [review]
[gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master

Please add r?=me once again when Bridge related PR is landed :)

Thanks!

Comment 18

[Steve Chung [:steveck]]

Comment on attachment 8638545 [details] [review]
[gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master

Hi, patch updated with timeout disabled feature(still in review). There should be no timeout for "no response" error but you can give it a try on your side.

Comment 19

[Julien Wajsberg [:julienw]]

Comment on attachment 8638545 [details] [review]
[gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master

r=me
I left some comments thought, so please fix them before landing:
* you forgot the desktop shim mock in your patch
* add the the iframe to views/inbox
* some style nits

Also I agree with Oleg's comments :)

Of course please wait for a green try before merging :)

Comment 20

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8638545 [details] [review]
[gaia] steveck-chung:new-message-messaging-service > mozilla-b2g:master

Looks good! Just left several nits at GitHub.

Thanks!

Comment 21

[Steve Chung [:steveck]]

Updated with the latest bridge library and the tree is green. Thanks for all the review!

In master: https://github.com/mozilla-b2g/gaia/commit/7afe8bd5d34b70a053620a2480df05c8f827bcec