Closed Bug 1069783 Opened 10 years ago Closed 7 years ago

[SMS] Draft is not saved while user click on link shared from browser for new thread

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: sasikala.paruchuri8, Unassigned)

References

Details

(Whiteboard: [LibGLA,Dev,WW, B] [g+][sms-most-wanted])

1. Title: Draft  is not saved while user click on link shared from browser for new thread
2. Precondition: Connect wi-fi and should be able to send and receive SMS
3. Tester's Action: 1.Open Browser application
                    2.Add some link 
                    3.select '->' in browser -> while browsing click on share
                    4.Select Messages -> Send the message to new number
                    5.Input some things in the composer
                    6.Click on the link

4. Actual Symptom (ENG.) : The message is not saved when user click on the link
5. Expected: The message should be saved as draft
Whiteboard: [LibGLA,Dev,WW, B] [g+]
As discussed on another bug, the issue here is that the SMS application is killed when starting the new activity, without having a chance to save the draft.

There are several possible ways to fix:

1. The window manager could hide the window for some time before killing it => this would trigger a visibilitychange event which would let us save the draft using the existing code.

2. We could save any current draft before starting any activity. I don't have a good idea on how to do this in a good maintainable way.

3. We can save the draft continuously (like we do on Tarako). Why not but this needs quite some new code.

NI alive for the possibility 1) which would be the easiest for us.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(alive)
See Also: → 1051719
(In reply to Julien Wajsberg [:julienw] from comment #1)
> As discussed on another bug, the issue here is that the SMS application is
> killed when starting the new activity, without having a chance to save the
> draft.
> 
> There are several possible ways to fix:
> 
> 1. The window manager could hide the window for some time before killing it
> => this would trigger a visibilitychange event which would let us save the
> draft using the existing code.

Well...I guess you cannot get any event or do anything if gecko already tells system it is killed.
If you want to give it a try:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L424
add |this.setVisible(false);| before the |this.destroy();|

BTW pardon me I don't understand this bug. Is there a background message app + foreground message activity?
Or browser app -> message activity -> browser app/activity?

> 
> 2. We could save any current draft before starting any activity. I don't
> have a good idea on how to do this in a good maintainable way.
> 
> 3. We can save the draft continuously (like we do on Tarako). Why not but
> this needs quite some new code.
> 
> NI alive for the possibility 1) which would be the easiest for us.
Flags: needinfo?(alive)
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #2)
> (In reply to Julien Wajsberg [:julienw] from comment #1)
> > As discussed on another bug, the issue here is that the SMS application is
> > killed when starting the new activity, without having a chance to save the
> > draft.
> > 
> > There are several possible ways to fix:
> > 
> > 1. The window manager could hide the window for some time before killing it
> > => this would trigger a visibilitychange event which would let us save the
> > draft using the existing code.
> 
> Well...I guess you cannot get any event or do anything if gecko already
> tells system it is killed.
> If you want to give it a try:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.
> js#L424
> add |this.setVisible(false);| before the |this.destroy();|

I'll have a look, or maybe sasikala can you try?
This looks somewhat racy still...

> 
> BTW pardon me I don't understand this bug. Is there a background message app
> + foreground message activity?
> Or browser app -> message activity -> browser app/activity?

yeah, last one.
browser app -> share url -> message -> send message -> click on url -> browser app
Hi Sasikala, following bug1051719, we continue the discussion here.
Could you please try comment3 suggested by Julien at your side? 
Really appreciate your support. Thank you very much.
Flags: needinfo?(sasikala.paruchuri8)
Hi Julien and Yang,

I have verified the changes suggested by Alive as below
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L424
> add |this.setVisible(false);| before the |this.destroy();|

We are not getting any visibilitychange event with these changes

Thanks!

(In reply to Rachelle Yang [:ryang][ryang@mozilla.com] from comment #4)
> Hi Sasikala, following bug1051719, we continue the discussion here.
> Could you please try comment3 suggested by Julien at your side? 
> Really appreciate your support. Thank you very much.
Flags: needinfo?(sasikala.paruchuri8)
Sasikala, can you try calling setVisible first, and then this.destroy() after 2 seconds (using setTimeout) and see how this behaves?
Flags: needinfo?(sasikala.paruchuri8)
If my memory serves me well, OOM killer will not even tell gecko when it'd like to kill the process.
Since the process is already killed, the running script will not get any event as well.

So I am afraid this does not work as expected. This is a platform issue we never figure out how to fix.
Yeah but we're not talking about the OOM killer here, but about the fact that the system app calls destroy() in this specific STR.

When we get OOM-killed we usually already got a visibilitychange event for other reason (except on Tarako, and that's why on Tarako we're saving continuously :) ).
(In reply to Julien Wajsberg [:julienw] from comment #3)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #2)
> > (In reply to Julien Wajsberg [:julienw] from comment #1)
> > > As discussed on another bug, the issue here is that the SMS application is
> > > killed when starting the new activity, without having a chance to save the
> > > draft.
> > > 
> > > There are several possible ways to fix:
> > > 
> > > 1. The window manager could hide the window for some time before killing it
> > > => this would trigger a visibilitychange event which would let us save the
> > > draft using the existing code.
> > 
> > Well...I guess you cannot get any event or do anything if gecko already
> > tells system it is killed.
> > If you want to give it a try:
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.
> > js#L424
> > add |this.setVisible(false);| before the |this.destroy();|
> 
> I'll have a look, or maybe sasikala can you try?
> This looks somewhat racy still...
> 
> > 
> > BTW pardon me I don't understand this bug. Is there a background message app
> > + foreground message activity?
> > Or browser app -> message activity -> browser app/activity?
> 
> yeah, last one.
> browser app -> share url -> message -> send message -> click on url ->
> browser app

Ok, but why do we need to save "draft" if we already sent the message in this process?
Because the user could have started a new message before clicking on the link.

In comment 6, I just want to investigate how a fix for System could look like and if it would be acceptable by you. But maybe the good solution is the proposal 2 or 3 (in comment 0). I want to know what works before choosing :)
Julien, I have checked adding 2sec delay for this.destroy() and we are receiving the visibilitychange event in this case :)

Added Code:

   else {
      this.setVisible(false);
      setTimeout(this.destroy, 2000);
    }

I just want to know information that when we click on the link is this the correct place where destroy is called?
Flags: needinfo?(sasikala.paruchuri8)
Alive, what do you think of the fix in comment 11? Is it acceptable or is it prone to races?

(In reply to sasikala from comment #11)
> Julien, I have checked adding 2sec delay for this.destroy() and we are
> receiving the visibilitychange event in this case :)
> 
> Added Code:
> 
>    else {
>       this.setVisible(false);
>       setTimeout(this.destroy, 2000);
>     }
> 
> I just want to know information that when we click on the link is this the
> correct place where destroy is called?

Yes, when we click on the link, and (I think) if we're in a cyclic activity, then the activity is removed.
Flags: needinfo?(alive)
Personally I don't want to introduce a timer anymore.
And this may not help if gecko does not send the event during the 2000ms - I mean we should not spend time on tuning the timer.

I could r+ only if we have no other way AND this had been completely tested - it affects all windows.
Flags: needinfo?(alive)
OK then, from the SMS point of view, let's do solution 3 from comment 1. It needs a little bit of work...
Hi Julien , thanks for feedback. Will you lease help on the solution 3 from comment 1?
Thank you very much.
Flags: needinfo?(felash)
We can help, bug it won't be picked up before next sprint.

If you want that we pick it up earlier, please ask for a blocker status. FTR I don't think this is a blocker: this is an edge case that the user will unlikely experience.

Moreover it requires quite a bit of work to fix this edge case and I think our time can be used in a better way. Please reach EPM or our product owner (Wilfred) if you want to bump the priority a little more.
Flags: needinfo?(felash)
Whiteboard: [LibGLA,Dev,WW, B] [g+] → [LibGLA,Dev,WW, B] [g+][sms-most-wanted]
See Also: → 1073232
Maybe we can ignore this issue once bug 1073232 fixed. In bug 1073232 we will exist from the composer activity after send button clicked, which means there will be no way to redirect to browser in activity. Please verify this issue after bug 1073232 landed.
Mass closing of Gaia::SMS bugs. End of an era :(
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Mass closing of Gaia::SMS bugs. End of an era :(
You need to log in before you can comment on or make changes to this bug.