Closed Bug 1058459 Opened 6 years ago Closed 5 years ago

[SMS] Data shared from other applications are not shown as Drafts until the app is killed and open again

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.0M+, b2g-v2.0 affected, b2g-v2.0M verified, b2g-v2.1 affected, b2g-v2.2 verified)

RESOLVED FIXED
2.1 S6 (10oct)
blocking-b2g 2.0M+
Tracking Status
b2g-v2.0 --- affected
b2g-v2.0M --- verified
b2g-v2.1 --- affected
b2g-v2.2 --- verified

People

(Reporter: sasikala.paruchuri8, Assigned: azasypkin)

References

Details

(Whiteboard: [g+][LibGLA,TD92050,QE1, B][sms-most-wanted][p=2])

Attachments

(8 files, 1 obsolete file)

1. Title: The data shared from other applications are not saved as Drafts from second time
2. Precondition: Store some images in Gallery application
3. Tester's Action: 1.Open Gallery application
                    2.Share 1 or 2 images to SMS application 
                    3.Select close button -> Select 'Save as Draft'
                    4.Open SMS application again 
                    5.Check the thread_list

4. Actual Symptom (ENG.) : The information is not shown in thread_list, but we can see the message saved when we restart the SMS application
5. Expected: The draft messages should be shown and saved everytime
The following error is shown during this scenario. 
08-26 12:19:23.899: E/GeckoConsole(22316): [JavaScript Error: "Error: Panel activity-share is unknown." {file: "app://sms.gaiamobile.org/js/navigation.js" line: 159}]

Steve, the scenario which we are discussing in https://bugzilla.mozilla.org/show_bug.cgi?id=1051719#c25 is also same problem. 

I think if we solve this issue -> the scenario in Bug 1051719 will be solved.

Kindly provide your opinion on this
Whiteboard: [g+]
Summary: [SMS] Data shared from other applications are not saved as Drafts → [SMS] Data shared from other applications are not shown as Drafts until the app is killed and open again
(In reply to sasikala from comment #0)
> 1. Title: The data shared from other applications are not saved as Drafts
> from second time
> 2. Precondition: Store some images in Gallery application
> 3. Tester's Action: 1.Open Gallery application
>                     2.Share 1 or 2 images to SMS application 
>                     3.Select close button -> Select 'Save as Draft'
>                     4.Open SMS application again 
>                     5.Check the thread_list
> 
> 4. Actual Symptom (ENG.) : The information is not shown in thread_list, but
> we can see the message saved when we restart the SMS application
> 5. Expected: The draft messages should be shown and saved everytime

Yeah, this happens because you have two active SMS app instances at the same time, one is that you opened by yourself, the second one is opened as inline activity by Gallery\Contacts app. We store drafts in asyncStorage and don't sync it between app instances for now. Perhaps we already had such bugs, I'll to try to find it.

> The following error is shown during this scenario. 
> 08-26 12:19:23.899: E/GeckoConsole(22316): [JavaScript Error: "Error: Panel
> activity-share is unknown." {file:
> "app://sms.gaiamobile.org/js/navigation.js" line: 159}]
> 
> Steve, the scenario which we are discussing in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1051719#c25 is also same
> problem. 
> 
> I think if we solve this issue -> the scenario in Bug 1051719 will be solved.
> 
> Kindly provide your opinion on this

This error is coming from navigation.js when it tries to parse fake "activity-share" URL used for share inline activity. This error is expected and shouldn't lead to any problems.
Whiteboard: [g+] → [g+][LibGLA,TD92050,QE1, B]
Steve and Oleg, will you please let us know is there any other way other than datastore for fixing these issues. Because there are many scenarios which occurs due to this like
1. Share image from Gallery -> Save as Draft
2. From contact -> select SMS icon -> Save as Draft
Flags: needinfo?(schung)
Flags: needinfo?(azasypkin)
Hi Oleg, since we didn't have any formal mechanism for inter-app communication now, I got a workaround for this issue: render the draft again while visibility change. Do you have other idea for addressing this issue?
Flags: needinfo?(schung)
(In reply to Steve Chung [:steveck] from comment #4)
> Hi Oleg, since we didn't have any formal mechanism for inter-app
> communication now, I got a workaround for this issue: render the draft again
> while visibility change. Do you have other idea for addressing this issue?

Yeah, this should work, but here we have to be smart enough, load drafts ->update\replace existing (if needed) and add new (our logic already supports correct positioning in the thread list). I've played a bit with refreshing thread node in the scope of "cache-first-page-html-to-improve-performance" experiment - that's doable.

Another "hacky" solution that I can think of is using "localStorage"+"window.onstorage" as event bus between active app instances.
Flags: needinfo?(azasypkin)
(In reply to Oleg Zasypkin [:azasypkin] from comment #5)

> Another "hacky" solution that I can think of is using
> "localStorage"+"window.onstorage" as event bus between active app instances.

Since onstorage event is related to localStorage and sessionStorage, not sure if it's still working on indexedDB as well(our asyncStorage is based on indexedDB). And not sure if this event could cross different window either.
(In reply to Steve Chung [:steveck] from comment #6)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #5)
> 
> > Another "hacky" solution that I can think of is using
> > "localStorage"+"window.onstorage" as event bus between active app instances.
> 
> Since onstorage event is related to localStorage and sessionStorage, not
> sure if it's still working on indexedDB as well(our asyncStorage is based on
> indexedDB). And not sure if this event could cross different window either.

Yeah our asyncStorage doesn't fire any events (that is bad I think), but I mean a really "hacky" solution where exactly "localStorage" is used, but _only_ as event bus :) See below:

// Eg. in startup.js
window.addEventListener('storage', function(e) {
  if (e.key === 'draft-saved') {
    // check if we have this draft displayed
    alert('Draft Id: ' + e.newValue);
  }
});

// And when draft is saved or anything other that requires cross instance sync, "key" is like event name, "value" is like event object or additional data
window.localStorage.setItem('draft-saved', draft.id);

Aslo we can have more pleasant shim-wrapper for that and replace internals once platform provides native support.
Duplicate of this bug: 1061986
ni Steve

Hi Steve, just wonder do you have a expected fix date for this issue?

Thanks

Vance
Flags: needinfo?(schung)
Hi Vince, we didn't put this issue for current sprint since it not blocker right not, so we will set it for next sprint if not urgent. Please ping me if you have any concern about the schedule, thanks!
Flags: needinfo?(schung)
Target Milestone: --- → 2.1 S5 (26sep)
Steve, Is it possible to complete this for current sprint?
and is there any work in progress patch for this?
In the meantime attaching small proof of concept with localStorage used as communication bus between app instances. Just in case it can give us some hints for ideal solution :)
Hey Sasikala,

Could you please help us to test PoC in attachment 8486260 [details] and say if it works for you?

I haven't tested it much yet, so any feedback is welcomed!

Thanks!
Flags: needinfo?(sasikala.paruchuri8)
Hi Oleg,

Thank you for the patch.I have tested the code from https://github.com/azasypkin/gaia/commit/8c40f94f2a13fcf5ad0f89e889fdb07a8af68c22

All the scenarios are working fine. Following are the scenarios tested
1. Open any contact from contact list -> select message icon -> click on 'X' button -> Save as Draft
   Result : The given contact is saved as Draft
2. Open camera -> take picture -> click on share button -> Select 'X' button -> Save as Draft
   Result: The image taken is saved
3. Open Gallery app -> Select image -> Slect 'X' button -> Save As Draft 
   Result: The image is saved
4. In SMS app -> Enter number -> enter text -> Select Back button -> Save As draft 
   Result : the entered text and number are saved

Doubt in below case:
1. Open Browser -> Share any link through Messages -> send -> Enter some text in same thread -> Select url-link -> Open SMS app again -> the entered text disappears

Not sure whether this case will be covered with this patch. Please check and provide your opinion on this.

Thanks!
Flags: needinfo?(sasikala.paruchuri8)
(In reply to sasikala from comment #14)
> Hi Oleg,
> 
> Thank you for the patch.I have tested the code from
> https://github.com/azasypkin/gaia/commit/
> 8c40f94f2a13fcf5ad0f89e889fdb07a8af68c22
> 
> All the scenarios are working fine. Following are the scenarios tested
> 1. Open any contact from contact list -> select message icon -> click on 'X'
> button -> Save as Draft
>    Result : The given contact is saved as Draft
> 2. Open camera -> take picture -> click on share button -> Select 'X' button
> -> Save as Draft
>    Result: The image taken is saved
> 3. Open Gallery app -> Select image -> Slect 'X' button -> Save As Draft 
>    Result: The image is saved
> 4. In SMS app -> Enter number -> enter text -> Select Back button -> Save As
> draft 
>    Result : the entered text and number are saved

Thanks for verifying!

> Doubt in below case:
> 1. Open Browser -> Share any link through Messages -> send -> Enter some
> text in same thread -> Select url-link -> Open SMS app again -> the entered
> text disappears
> 
> Not sure whether this case will be covered with this patch. Please check and
> provide your opinion on this.
> 
> Thanks!

Hmm, interesting case... Please, confirm that I understood your case correctly:

1. You opened Messages app instance #1 (later just #1) from HomeScreen and navigated to the Thread "A";
2. Then you opened Browser app, shared URL with "A" via another Messages app instance #2 (later just #2, it was opened as inline activity);
3. Now you have two active Messages app instances - #1 and #2 (you can check via Task Manager);
4. In #2 you entered text in message input, taped on link and was redirected to Browser App (#2 changed visibility to hidden and autosaved draft, this change was propagated to both #1 and #2 Thread _List_ panels);
5. Now you still have two active Messages app instances.
6. Then from HomeScreen you opened #1 and obviously you didn't see anything in message input :)

So we can potentially refresh message input in #1 Thread panel with the latest draft, but it looks confusing to me as user. What if I already started writing message in #1? So I have two different drafts in message input for #1 and #2. To me the less error prone solution would be just closing #2 (leave inline activity) once message is sent, but it's more UX call.

Steve, Sasikala what do you think about it?

Thanks!
Flags: needinfo?(schung)
Flags: needinfo?(sasikala.paruchuri8)
Hi Oleg,

Sorry for confusion in  the scenario. Please see the detailed steps

1. Open Browser app -> Enter any link -> Share the link through Messages app(Not opened SMS app before)
2. Input some number in 'To' field -> Send the message
3. Open the same thread from where the message was sent -> enter some text in composer field
4. Then click on the link
5. Open the SMS app and verify -> the message entered does not exist.

There is only one message app instance in this case

Thanks!
Flags: needinfo?(sasikala.paruchuri8)
Hi Sasikala,

How do you get to Messages app in step #5? Do you swipe back from Browser app (Messages inline activity is still active after step #4) or you press Home and run new Messages app from HomeScreen.

Anyway seems I can't reproduce it: if I swipe back to the first instance I see entered message in message input, if I go to HomeScreen and run new app, I see that draft in Thread List panel too... 

I can imagine only one case when this can happen - if user has very slow device and asyncStorage actually saved draft after you opened new Messages app (but that should eventually work if you wait a bit before opening Messages app again, I have idea how to improve this though) or first app was even killed before it saved draft. But I can't reproduce it on my Flame.

You can ping me on IRC to investigate it quickly :)
Flags: needinfo?(schung) → needinfo?(sasikala.paruchuri8)
Added PR link instead, to make tracking changes\feedback easier.
Attachment #8486260 - Attachment is obsolete: true
Clearing the needinfo as the scenario is already discussed on IRC. The below scenario is only reproduced in 2.0 branch

1. Open Browser app -> Enter any link -> Share the link through Messages app(Not opened SMS app before)
2. Input some number in 'To' field -> Send the message
3. Open the same thread from where the message was sent -> enter some text in composer field
4. Then click on the link
5. Open the SMS app and verify -> the message entered does not exist.
Flags: needinfo?(sasikala.paruchuri8)
Hi Oleg, if we really need to communicate cross instance, why not use postMessage directly? Since we  don't really need to safe anything at all, and having additional localStorage might lead load time penalty.
Flags: needinfo?(azasypkin)
(In reply to Steve Chung [:steveck] from comment #20)
> Hi Oleg, if we really need to communicate cross instance, why not use
> postMessage directly? Since we  don't really need to safe anything at all,
> and having additional localStorage might lead load time penalty.

Hmm, don't remember honestly, maybe because 'storage' is not fired in the source window that made PoC simpler :) But agree if postMessage works as expected then I think is it's much better than hacky localStorage solution.

Sasikala, could you please help to modify PoC to use postMessage instead?

Thanks!
Flags: needinfo?(azasypkin) → needinfo?(sasikala.paruchuri8)
Sorry, just realized that to use window.postMessage we have to have reference to the target window, but we don't have any available relationship between main app window and one that belongs to inline activity app.

Or am I missing something?
Flags: needinfo?(sasikala.paruchuri8) → needinfo?(schung)
Hmm, not sure if we simply use '*' (no preference) could still work. I also found some app use
  document.location.protocol + '//' + document.location.host
to represent their original url(maybe using mozApp could only get the parent window). But in this case I think even using hard-coded message app url is not that bad because we only need to dispatch the message to main app window.
Flags: needinfo?(schung)
Sorry still didn't get how from activity window (#2) we can directly get window for the main app (#1) to post message there. Simple code snippet would help a lot :)

──iframe[src=system.gaiamobile.org] (System Main Window)
   │
   ├──div.appwindow
   │    │
   │    └──iframe[src=sms.gaiamobile.org] (Messages Main Window #1)
   │    
   └──div.appwindow
        │
        ├──iframe[src=gallery.gaiamobile.org] (Gallery Main Window)
        │
        └──iframe[src=sms.gaiamobile.org] (Messages Inline Activity Window #2)

Thanks!
Comment on attachment 8487060 [details] [review]
GitHub pull request URL (master)

Hey Steve,

I've just added to PoC one more option for inter instance communication - via SharedWorker. IMO it's still hack, but via another feature :)

So would be happy to get your feedback and thoughts on two current approaches!

Thanks!
Attachment #8487060 - Flags: feedback?(schung)
Duplicate of this bug: 1066949
Assignee: nobody → azasypkin
Status: UNCONFIRMED → NEW
Ever confirmed: true
SharedWorker could be more future-proof because we'll eventually use ServiceWorker and this looks like this would work well together?
Comment on attachment 8487060 [details] [review]
GitHub pull request URL (master)

I think there is no proper solution than SharedWorker here, it's really a great idea :) Just left a concern in github, but I think we should take this eventually.
Attachment #8487060 - Flags: feedback?(schung) → feedback+
Hey Andrea,

I believe you're the right person to give an expert advice here :)

In Messages (Firefox OS Gaia app) we have use case when several iframes (with the same origin) that don't have direct links to each other should communicate. Basically it's two Messages app instances. We found out only the following ways to do so.

Could you please help to answer questions we have regarding to them and maybe you have better idea?

1. BroadcastChannel API (Bug 966439). Looks like it's the best and only correct option for this use case, but it's still in progress.

So the question is: do you have any approximate timelines when BroadcastChannel API is going to be landed? Is it correct to use it for our use case?

2. SharedWorker (hack). We use SharedWorker as mediator event bus between these iframes that broadcasts messages to the connected ports. The problem here is to somehow close and remove references to "dead" ports. We had two solutions for that:
  1. Once we send message to port we always wait for some sort of "ack" reply and close\remove reference to the port if we don't get reply.
  2. Relying on window.onunload to close SharedWroker port.
Both aren't ideal as seems we have issues with unload\beforeunload events for iframe in Gaia. 

Question here is what consequences we'll have if we keep references to that "dead" ports and keep posting messages to it? Will it be GC'ed correctly when all iframes are destroyed?

3. LocalStorage (hack). We use LocalStorage & window.onstorage as mediator event bus between these iframes. Don't have questions about it, just noted for the full picture :)

Thanks!
Flags: needinfo?(amarchesini)
BroadcastChannel does seem like a great fit here but there's an unknown amount of risk to rushing to finish that and pref it on for B2G only.  I'm not the best person to make the risk vs reward decision but if it were up to me I'd lean towards the SharedWorker hack as ugly as it may be.
I seem to remember that our current BroadcastChannel implementation doesn't work across processes, so I don't think it will solve the use case described in comment 29 (Andrea can correct me if I'm wrong there.)
Don't all windows for an app live in the same process? Or maybe this changed recently?
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO on Thu/Fri) from comment #31)
> I seem to remember that our current BroadcastChannel implementation doesn't
> work across processes, so I don't think it will solve the use case described
> in comment 29 (Andrea can correct me if I'm wrong there.)

I think it's better if I write a test for this, but I guess it works across processes because the all system is based on IPC and PBackground. It means that when an BroadcastChannel object is created, the child lives on the thread X, process Y, but the parent actor lives in the main-process.

Yes, I guess it should work. I'll write a mochitest to cover this scenario.

About comment 29:

BroadcastChannel API is what you want. But this cannot land for 2.1 for a couple of reasons:

1. is not fully reviewed yet.
2. it's a new API and it must be tested deeply.

For sure, the localStorage solution is a big "no way". but the SharedWorker can work. In theory until there is 1 window pointing to the SharedWorker, this worker is kept alive so you should not see 'dead' ports.

I can imagine that this protocol between iframe, based on SharedWorker can be tricky and fully of race conditions, but I think it's the only solution you have at the moment.

Question: why you don't have a reference between the iframes?
If you have it you can use MessagePort/MessageChannel.
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #33)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO on
> Thu/Fri) from comment #31)
> > I seem to remember that our current BroadcastChannel implementation doesn't
> > work across processes, so I don't think it will solve the use case described
> > in comment 29 (Andrea can correct me if I'm wrong there.)
> 
> I think it's better if I write a test for this, but I guess it works across
> processes because the all system is based on IPC and PBackground. It means
> that when an BroadcastChannel object is created, the child lives on the
> thread X, process Y, but the parent actor lives in the main-process.
> 
> Yes, I guess it should work. I'll write a mochitest to cover this scenario.
> 
> About comment 29:
> 
> BroadcastChannel API is what you want. But this cannot land for 2.1 for a
> couple of reasons:
> 
> 1. is not fully reviewed yet.
> 2. it's a new API and it must be tested deeply.

From the Mozilla point of view, we don't want to land on v2.1. But maybe the partners need it though, so we should probably stick to Shared Worker as you suggest.

> 
> For sure, the localStorage solution is a big "no way". but the SharedWorker
> can work. In theory until there is 1 window pointing to the SharedWorker,
> this worker is kept alive so you should not see 'dead' ports.

The dead port could be inside the SharedWorker:
* the main application connects, the shared worker gets a "onconnect" event and store the port
* the activity window connects, the shared worker gets a "onconnect" event and store the port
* the main window is closed.... what happens ?

> 
> I can imagine that this protocol between iframe, based on SharedWorker can
> be tricky and fully of race conditions, but I think it's the only solution
> you have at the moment.
> 
> Question: why you don't have a reference between the iframes?
> If you have it you can use MessagePort/MessageChannel.

They're both launched by the System app, we don't have any reference from inside the application. One is the main application windows, the other is an activity.

Hope this is clearer !
Flags: needinfo?(amarchesini)
> The dead port could be inside the SharedWorker:
> * the main application connects, the shared worker gets a "onconnect" event
> and store the port
> * the activity window connects, the shared worker gets a "onconnect" event
> and store the port
> * the main window is closed.... what happens ?

That is totally fine:
https://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#5037

Probably you will leak a port as this small example does:

var ports = [];
onconnect = function(evt) { ports.push(evt.ports[0]); }

setInterval(function() {
  for (var i = 0; i < ports.length; ++i) {
    ports[i].postMessage("Ports: " + ports.length + " = " + (new Date()).toString());
  }
}, 1000);

As you can see, just with this piece of code, you don't have any way to know if the message has been deliverered or not because postMessage doesn't throw an exception if the port has been closed on the main-thread side.

Would be nice if you file a bug about using BroadcastChannel API. So we can test this API with something real when it lands.
Flags: needinfo?(amarchesini)
For your info - 

First I applied this patch then I applied this patch https://github.com/mozilla-b2g/gaia/pull/24145

Following are the Scenarios fails-

1.	Save some contact -> Select Message icon from contact -> Select ‘x’ button -> ‘Save as Draft’
Result: The Draft message is not saved
2.	Enter some number in dialer -> Select Message icon -> Select ‘x’ button -> ‘Save as Draft’
Result : Draft is not saved

Out of curiosity I checked it on Moz v2.0.

Thanks
Ankit, do you see the draft once you kill the application and start it again? If yes, then this is exactly what we're trying to fix here.
Julien, Able to see the draft messages after killing the application with these changes but there is some race condition where even after saving the draft it is not saved(Save some contact -> Select Message icon from contact -> Select ‘x’ button -> ‘Save as Draft’) and we can observe the same draft after restarting the application. Not sure about the exact STR for this case

(In reply to Julien Wajsberg [:julienw] from comment #37)
> Ankit, do you see the draft once you kill the application and start it
> again? If yes, then this is exactly what we're trying to fix here.
Sasikala, could be bug 1061417. It would be easier if you'd share your code ;)
Julien,

For Info: I have applied both the patches from https://bugzilla.mozilla.org/show_bug.cgi?id=1061417 and bug 1058459

Below are the observations for the issue.
STR for the issue:
1. Follow all the steps from https://bugzilla.mozilla.org/show_bug.cgi?id=1061417
2. Add contact -> Select contact -> click on message icon -> Slect 'x' -> Save as Draft
  Result: The draft is not saved
3. Take a picture from camera -> share the images to Message -> Slect 'x' -> Save as Draft
  Result: Draft is not saved
Observations: We can see the saved drafts after restarting the message application
Expected : The draft should be saved in all the scenarios.

Thanks,

(In reply to Julien Wajsberg [:julienw] from comment #39)
> Sasikala, could be bug 1061417. It would be easier if you'd share your code
> ;)
> Observations: We can see the saved drafts after restarting the message application

=> this means it _is_ saved

What you see is exactly this bug. The patch we have here is not ready for prime time yet.
Hi Steve

Precondition: We merged this above reviewed code.

Test 1: I merged this https://github.com/mozilla-b2g/gaia/pull/24145/files
   Issue - Save some contact -> Select Message icon from contact -> Select ‘x’ button -> ‘Save as Draft’
Result: The Draft message is not saved

Test 2: I tried with https://github.com/mozilla-b2g/gaia/commit/d6662ca19da9d73456f04d7b03f154d1560a7acf
   Issue - Save some contact -> Select Message icon from contact -> Select ‘x’ button -> ‘Save as Draft’
Result: The Draft message is not saved

Please we need see this.. 

Thanks a lot!
Flags: needinfo?(schung)
Flags: needinfo?(azasypkin)
Steve and Oleg are busy on other bugs for now.

We'll take this bug next sprint.
For the story: we discussed about taking the bug for this sprint, but thought that you could finish the bug by yourself (either ankit or sasikala) using the localStorage solution. In the mean time we now think the Shared Worker solution is better, but since it's more complicated I think it's better we do it in Mozilla.

That means the bug would be ready 14th October at the latest (probably earlier though). I hope it's ok for you.

If you want to change the priority of this bug, please make it a blocker.
Flags: needinfo?(schung)
Flags: needinfo?(azasypkin)
Duplicate of this bug: 1069843
Luke, can I please access that bug?
Flags: needinfo?(lchang)
Hi Julien, Sure! CCed.
Flags: needinfo?(lchang)
Hi Luke,since this bug1058459 is a partner blocker at this stage.
If possible, may I know the progress of Bug1069843 or get access to it if they are duplicate issues?
Thank you very much.
Flags: needinfo?(lchang)
See comment 43, there is nothing new :)

The next sprint starts tomorrow.
Flags: needinfo?(lchang)
Whiteboard: [g+][LibGLA,TD92050,QE1, B] → [g+][LibGLA,TD92050,QE1, B][sms-most-wanted]
Hi Rachelle, CC'ed but there is no progress in that bug actually.
Whiteboard: [g+][LibGLA,TD92050,QE1, B][sms-most-wanted] → [g+][LibGLA,TD92050,QE1, B][sms-most-wanted][p=2]
Target Milestone: 2.1 S5 (26sep) → 2.1 S6 (10oct)
Status: NEW → ASSIGNED
Comment on attachment 8487060 [details] [review]
GitHub pull request URL (master)

Hey Steve,

I think the PR is ready for the first round of review, I've added some logic to clear dead port references + a bunch of unit tests. 

Could you please review it?


Thanks!
Attachment #8487060 - Flags: review?(schung)
Comment on attachment 8487060 [details] [review]
GitHub pull request URL (master)

Some feedbacks there but mostly looks great, the only major concern is the worker postMessage parameter. About the unexpected draft saving issue, it seems not a regression per my testing. So maybe we could address this in other bug if you could confirm this as well.
Attachment #8487060 - Flags: review?(schung)
(In reply to Steve Chung [:steveck] from comment #51)
> Comment on attachment 8487060 [details] [review]
> GitHub pull request URL (PoC with localStorage)
> 
> Some feedbacks there but mostly looks great, the only major concern is the
> worker postMessage parameter. About the unexpected draft saving issue, it
> seems not a regression per my testing. So maybe we could address this in
> other bug if you could confirm this as well.

Thanks for review! I've filed bug 1079824 about duplicated drafts.
Comment on attachment 8487060 [details] [review]
GitHub pull request URL (master)

Hey Steve,

I've added your suggestions to the separate commit. Regarding to issue with save-discard draft from activity you've mentioned I'd rather do it in the separate bug as it doesn't sound really simple to resolve (will file a bug if you're ok with it).

Thanks!
Attachment #8487060 - Flags: review?(schung)
Comment on attachment 8487060 [details] [review]
GitHub pull request URL (master)

Only one small nit, so r=me. Now we can communicate across instance, and it's really nice :)
Attachment #8487060 - Flags: review?(schung) → review+
Blocks: 1080480
See Also: → 1080480
Blocks: 1080504
Thanks for review! Nit fixed and Ttreeherder is green. Also I've filed some follow-ups:

* Bug 1080480 - to migrate to BroadcastChannel API once it's ready;
* Bug 1080504 - for issue you've found with discarding drafts from activity;
* Bug 1079824 - to track IndexDB regression that led to duplicated drafts if saved from activity.

Master: https://github.com/mozilla-b2g/gaia/commit/33a481e5418a57ab8eef57dd21902b102d2f649c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1084176
Duplicate of this bug: 1098230
Attachment #8487060 - Attachment description: GitHub pull request URL (PoC with localStorage) → GitHub pull request URL (master)
Rebased PR for v2.0m branch
Blocks: Woodduck
blocking-b2g: --- → 2.0M+
Duplicate of this bug: 1112959
Hi Oleg,
Partner will verify again after they have the patch on 2.0M from us. Can you raise review request for 2.0M patch?
Thank you very much!
Flags: needinfo?(azasypkin)
Comment on attachment 8540126 [details] [review]
GitHub pull request URL (v2.0m)

Hey Steve,

As you reviewed master patch, could you please review v2.0m one? The "biggest" difference is n startup.js part only, the rest - is just rebased version of master patch.

Thanks!
Flags: needinfo?(azasypkin)
Attachment #8540126 - Flags: review?(schung)
Comment on attachment 8540126 [details] [review]
GitHub pull request URL (v2.0m)

Looks good, thanks for the rebasing!
Attachment #8540126 - Flags: review?(schung) → review+
Hi Kai-Zhen,
Could you help to land this on 2.0M? Thanks!
Flags: needinfo?(kli)
Attached video Verify_1 video
Hi Sasikala,
According to comment#14, the Case 1~4 are verified NOT to happen on latest Woodduck 2.0 and Flame2.2
See attachments: Verify_1.MP4 
Reproducing rate: 0/5

(In reply to sasikala from comment #16)
> 1. Open Browser app -> Enter any link -> Share the link through Messages
> app(Not opened SMS app before)
> 2. Input some number in 'To' field -> Send the message
> 3. Open the same thread from where the message was sent -> enter some text
> in composer field
> 4. Then click on the link
> 5. Open the SMS app and verify -> the message entered does not exist.

1. On Flame2.2, I swipe back from Browser app to enter Messages app in step #5
->The verified result: Text in composer field is saved; 
I press Home and run new Messages app from HomeScreen in step #5
->The verified result: Text in composer field is saved;

2. On Woodduck2.0, I can't swipe back from Browser app to enter Message app in step#5, so i only can press Home and run new Messages app from HomeScreen in step #5
->The verified result is Failed: Text in composer field can't be saved;
See attachments: Verify_2.MP4 & Woodduck_logcat_1137.txt
Reproducing time: 11:37
Reproducing rate: 5/5

Could you help to confirm whether the STR in my video is correct? 
Thanks!

Woodduck 2.0 build:
Gaia-Rev        fd0029522b8804319ab1174ebcf4b73c147bef8b
Gecko-Rev       bb95bcf5c2033a59b3261b93804151d1addb75eb
Build-ID        20150105050313
Version         32.0

Flame 2.2 build:
Gaia-Rev        c2bf20d23851d5fda9f8f0ef0267db5f49152376
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/55f3224d7513
Build-ID        20150104010206
Version         37.0a1
Flags: needinfo?(sasikala.paruchuri8)
Attached file logcat
(In reply to Shine from comment #65)
> Created attachment 8543793 [details]
> Verify_1 video
> 
> Hi Sasikala,
> According to comment#14, the Case 1~4 are verified NOT to happen on latest
> Woodduck 2.0 and Flame2.2
> See attachments: Verify_1.MP4 
> Reproducing rate: 0/5
> 
> (In reply to sasikala from comment #16)
> > 1. Open Browser app -> Enter any link -> Share the link through Messages
> > app(Not opened SMS app before)
> > 2. Input some number in 'To' field -> Send the message
> > 3. Open the same thread from where the message was sent -> enter some text
> > in composer field
> > 4. Then click on the link
> > 5. Open the SMS app and verify -> the message entered does not exist.
> 
> 1. On Flame2.2, I swipe back from Browser app to enter Messages app in step
> #5
> ->The verified result: Text in composer field is saved; 
> I press Home and run new Messages app from HomeScreen in step #5
> ->The verified result: Text in composer field is saved;
> 
> 2. On Woodduck2.0, I can't swipe back from Browser app to enter Message app
> in step#5, so i only can press Home and run new Messages app from HomeScreen
> in step #5
> ->The verified result is Failed: Text in composer field can't be saved;
> See attachments: Verify_2.MP4 & Woodduck_logcat_1137.txt
> Reproducing time: 11:37
> Reproducing rate: 5/5
> 
> Could you help to confirm whether the STR in my video is correct? 
> Thanks!
> 
> Woodduck 2.0 build:
> Gaia-Rev        fd0029522b8804319ab1174ebcf4b73c147bef8b
> Gecko-Rev       bb95bcf5c2033a59b3261b93804151d1addb75eb
> Build-ID        20150105050313
> Version         32.0
> 
> Flame 2.2 build:
> Gaia-Rev        c2bf20d23851d5fda9f8f0ef0267db5f49152376
> Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/55f3224d7513
> Build-ID        20150104010206
> Version         37.0a1

Hi Shine,

I have seen the videos shared in attachments(Verify_1.MP4 & Verify_2.MP4) and the procedure(STR to reproduce the issues) which is followed in video is correct.

Thanks!
Flags: needinfo?(sasikala.paruchuri8)
Hi peipei,
Per comment#65, this problem is verified to fail on Woodduck2.0, Could you help with it, thanks!
Repro steps:
1. Open Browser app -> Enter any link -> Share the link through Messages app (Not opened SMS app before)
2. Input some number in 'To' field -> Send the message
3. Open the same thread from where the message was sent -> enter some text in composer field 
4. Then click on the link
5. Press Home key and launch Messages app from HomeScreen -> Open the same thread 
** The message entered does not exist.
Flags: needinfo?(pcheng)
Shine, could you please test on latest woodduck build again? I'm not sure if your build 20150105050313 contains the commit.
Flags: needinfo?(pcheng) → needinfo?(yue.xia)
Attached file woodduck logcat
Hi peipei,
This problem can be repro on latest Woodduck2.0.
See attachments: Video_woodduck.MP4 & logcat_1407.txt
Reproducing time: 14:07
Reproducing rate: 5/5

Woodduck 2.0 build:
Gaia-Rev        06b544f8a9ee03754dd3f5020f279cffa7a75804
Gecko-Rev       6b1e24a1cba1d5542bebb503ce05a1ec61ceb7d3
Build-ID        20150107050313
Version         32.0
Flags: needinfo?(yue.xia)
Please note, that the issue from comment 69 is different from one fixed in this bug, see bug 1069783 for more details
Blocks: 1080481
Attached video Verify2 video
Per Comment 73, this problem is verified pass with the STR in Comment 0 on latest Woodduck2.0
See attachments: Verify2_video.MP4
Rate: 0/5

Woodduck 2.0 build:
Gaia-Rev        22e3b7855281aa7b6190e01b4bd50c79880a1e6a
Gecko-Rev       20943fe7b54c63a375952fbd8dd396a22ee893e7
Build-ID        20150116050313
Version         32.0
Device-Name     jrdhz72_w_ff
FW-Release      4.4.2
FW-Incremental  1421355897
FW-Date         Fri Jan 16 05:05:17 CST 2015
Blocks: 1140339
Duplicate of this bug: 1159125
You need to log in before you can comment on or make changes to this bug.