Julien Wajsberg [:julienw] Reporter
	Description • 9 years ago

This is a regression of bug 1162030.

STR:
1. Receive a SMS with a notification (means you should not receive a SMS for an opened conversation).
2. Force-kill the app (or reboot the phone).
3. Click the notification.

Expected:
* The Conversation panel is opened asap. If possible we should not see the Inbox at all, but seeing it a bit without conversations is good enough for this bug.

Actual:
* We see the Inbox and we see conversations loading before the app switches to the Conversation panel.


While fixing this bug, we need to take care the following STR still works (I believe there is an integration tests for this):
1. Receive a SMS with a notification (means you should not receive a SMS for an opened conversation).
2. Force-kill the app (or reboot the phone).
3. Dismiss the notification.
4. Launch the app.
=> It should load the Inbox just fine.

	Julien Wajsberg [:julienw] Reporter
	Comment 1 • 9 years ago

[Blocking Requested - why for this release]:

	Julien Wajsberg [:julienw] Reporter
	Comment 2 • 9 years ago

In this bug I'd like that we think how Navigation and ActivityHandler work together.

Also it might be a good time to split the system messages work from the activity work, because they do quite different things and it might help later on with the split work too.

	Mahendra Potharaju [:mahe]
	Comment 3 • 9 years ago

Francisco, 

Can you please take a look at this and set a priority and owner?

Thanks

	Steve Chung [:steveck] Assignee
	Comment 4 • 9 years ago

Since Julien(In PTO) has already had some idea about this issue, and rest of peers are working on other NGA related tasks, we didn't put this blocker in this sprint. We will have some discussion and assign to the appropriate person in the next sprint planning, thanks.

	Francisco Jordano [:arcturus] [:francisco]
	Comment 5 • 9 years ago

Steve will take a look, but I think this is not a regression, is part of the on going work.

	Steve Chung [:steveck] Assignee
	Comment 7 • 9 years ago

Comment on attachment 8671193 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-fix > mozilla-b2g:master

Hi Julien,
It's the simplest solution by far I could think of and it shouldn't lead to other side effects. Since Oleg already introduced the idea that pending the inbox rendering after navigated event, the problem is we fired a navigated event while initialization as well. So the solution here is only rename the event fired when initialized, or we could simply remove the event while initialization because it seems not necessary for now. Tell me if you think this idea is inappropriate, thanks.

	Julien Wajsberg [:julienw] Reporter
	Comment 8 • 9 years ago

Comment on attachment 8671193 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-fix > mozilla-b2g:master

I don't think this fixes the issue we want to fix. We still enter the Inbox before going to the Conversation, and that's what we want to avoid IMO.

The change also has some subtle side-effects because the inbox is not rendered when we enter a conversation, until we navigate again (for example when we send a SMS to 2 people, we can see the inbox rendering before closing the app -- small annoyance though. Maybe other side-effects too but I don't really find some yet).

That's why I said I can't think of a simple and clean solution yet.

One possibility could be to use a "#notification" hash for this system message, but because of bug 818000 then we wouldn't get "sms-received" and other system messages... So this possibility is out.

Another possibility is not very clean, but this is the only one I can think of. In navigation.js, in startNavigationFromCurrentLocation(), we hardcode that if (navigator.mozHasPendingMessages('notification')), we return a rejected promise. So this would work the same than for activities where we have a #hash that has the same effect.

Tell me what you think !

	Steve Chung [:steveck] Assignee
	Comment 9 • 9 years ago

Comment on attachment 8671193 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-fix > mozilla-b2g:master

Ok, I misunderstood your expected result in description and I thought the avoiding the thread rendering might be enough :)

I updated the patch by checking the pending notification message, and yes it's obviously better than my original patch(although I'm not sure about the side effect you mentioned since I only rename the event when navigation initialized and we only need navigated event in startup). However I returned reject in init instead of in startNavigationFromCurrentLocation because I assume we only need check this at the navigation init step. I also tested about the performance impact about the checking and it should be quite safe(less then 10 ms in general). Thanks for the advise and please let me know if it might introduce any other side effect.

	Julien Wajsberg [:julienw] Reporter
	Comment 10 • 9 years ago

Comment on attachment 8671193 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-fix > mozilla-b2g:master

works perfect

And you're totally right it should be in init() :)

	Steve Chung [:steveck] Assignee
	Comment 11 • 9 years ago

Thanks for the integration test that let us found one serious issue in this patch: Removing the notification will also fire a system message to launch the message app with pending notification system message even the notification is removed actually... Should we ping system front-end team for the support(like disable the system message while removing the notification from long time) or find other solution?

	Oleg Zasypkin [:azasypkin]
	Comment 12 • 9 years ago

(In reply to Steve Chung [:steveck] from comment #11)
> Should we ping system front-end team for
> the support(like disable the system message while removing the notification
> from long time)

IIRC we've tried this already (see bug 1141014 and bug 1139363), but it requires quite a bit of work, that likely will not happen in v2.5 timeframe :/

	Julien Wajsberg [:julienw] Reporter
	Comment 13 • 9 years ago

I have a suggestion, tell me what you think.

IMO we have basically 2 choices:

choice 1: closing the window when receiving the 'notification' system message with 'clicked' false, but only if the window is hidden, and is not in the process of being open, and if we don't have other system messages waiting to be sent. (that's my biggest fear here, I suspect we could check it using good integration tests. Note I already have this fear with the implmentation we have for Network alerts.)

choice 2: displaying the inbox panel if "nothing" is displayed when we get a "visibilitychange" event with hidden = false.

In both cases we need to manage the race we have between:
* the 2 "notification" messages we get when the user taps on it
* any race with system messages + launch we could have.

So I'm thinking of involving the task runner here:

https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/views/shared/js/task_runner.js

With the task runner, we could make sure that the first system message is handled before the next one's handler starts. And then we could prevent races.

Maybe we'd need a new separate object (startup_manager -- unless it's handled in startup.js somehow ?) that would keep an instance of a task runner. Then the code in startup.js and the code in activity_handler and system_message_handler could contribute their startup tasks. And we'd be sure that no startup task run concurrently.


I'm thinking of this for a long time, IMO this is the only clean way to have a not racy startup. All other things we did so far were hacky and fragile things.


What do you think, Oleg and Steve ?

	Steve Chung [:steveck] Assignee
	Comment 14 • 9 years ago

I was not very sure about the window visibility while app received system message in each condition, so I made a small experiment:

- When click on the notification, there will be 2 message with click true and false consecutively. The visibility status for first message with click === true is hidden but the following message  with click === false is visible. Originally I thought these 2 message might have same visibility status, now we can know when to close the app if it's different.

- There's another interesting fact that the visibility in the navigation init is different while clicking on the notification. The visibility at the navigation init would be visible at first, then at the 1st system message it'll become hidden, and visible again when 2nd message comes.

So checking the visibility status might work, although I'm not confident about the visibility status in each state.

About the race, the only possible race I can think of is when we click on the notification. But I'm not sure it's necessary to put them into taskRunner. Assuming the visibility state is solid like I saw above, we can check the visibility directly. The situation we might need the taskRunner would be unconfirmed visibility status while handling first message.

And for the launch/system message race, I'm not sure where we'll need the taskRunner in system message launch case. System message handling part must start first and fires the navigated event for inbox view. The problem is we don't finish the navigation properly in system message handling.

So back to the original problem, do you think we can rely on the visibility and close the app if both click ===false/document hidden ?

	Julien Wajsberg [:julienw] Reporter
	Comment 15 • 9 years ago

(In reply to Steve Chung [:steveck] from comment #14)
> I was not very sure about the window visibility while app received system
> message in each condition, so I made a small experiment:
> 
> - When click on the notification, there will be 2 message with click true
> and false consecutively. The visibility status for first message with click
> === true is hidden but the following message  with click === false is
> visible. Originally I thought these 2 message might have same visibility
> status, now we can know when to close the app if it's different.

But I suspect there can be a race here. Basically, it's visible if the System app called "setVisible" on the Iframe, means after we run app.launch() and some other asynchronous things happen in the System app. So I'd rather not suppose it will be always the same and I prefer to make sure we're in a consistent state by sequencing the calls.


> 
> - There's another interesting fact that the visibility in the navigation
> init is different while clicking on the notification. The visibility at the
> navigation init would be visible at first, then at the 1st system message
> it'll become hidden, and visible again when 2nd message comes.

Are you sure this happens in this order? Because this looks very surprising.

> 
> So checking the visibility status might work, although I'm not confident
> about the visibility status in each state.
> 
> About the race, the only possible race I can think of is when we click on
> the notification. But I'm not sure it's necessary to put them into
> taskRunner. 

I'm also afraid of (for example) receiving a message while clicking a notification. This would be bad luck, but in the entire world bad luck happens.

> Assuming the visibility state is solid like I saw above, we can
> check the visibility directly. The situation we might need the taskRunner
> would be unconfirmed visibility status while handling first message.
> 
> And for the launch/system message race, I'm not sure where we'll need the
> taskRunner in system message launch case. System message handling part must
> start first and fires the navigated event for inbox view. The problem is we
> don't finish the navigation properly in system message handling.
> 
> So back to the original problem, do you think we can rely on the visibility
> and close the app if both click ===false/document hidden ?

As I said, I'm very worried that "document hidden" is not constant, depending on the device, on the load, on anything really. I'm worried this can change if the System app changes something in their window manager, I'm worried this can change if Gecko changes something in their System message handling. I don't think we can rely on this if we don't sequence properly.

What do you think ?

	Steve Chung [:steveck] Assignee
	Comment 16 • 9 years ago

Comment on attachment 8671193 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-fix > mozilla-b2g:master

Update the patch with considering the navigation state. I agree that considering the visibility is relatively fragile because any system implementation changes will break our app unexpectedly. I added an isValidCurrentState method because it will be true only after valid initialization or navigation. I think we can check this as well instead of visibility only, because message app will be hidden when we open a picker in the foreground(that reminds me that app was visible previously when in background).

And about the visibility state in navigation.ini() -> 1st notification message -> 2nd notification message. I confirmed again today and it's visible -> hidden -> visible for sure. And yeah it looks strange for me as well...

	Julien Wajsberg [:julienw] Reporter
	Comment 17 • 9 years ago

I tried myself, and I get:

Navigation.init, hidden= true 
system message handler init, hidden= true 
onNotificationSystemMessage, clicked=true, hidden=true 
onNotificationSystemMessage, clicked=false, hidden=false 


My STR is:
1. receive a message
2. force kill the messages app
3. tap the notification


This is with latest Flame build.

	Julien Wajsberg [:julienw] Reporter
	Comment 18 • 9 years ago

Comment on attachment 8671193 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-fix > mozilla-b2g:master

More comments and suggestions on github.

	Steve Chung [:steveck] Assignee
	Comment 20 • 9 years ago

Comment on attachment 8671193 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-fix > mozilla-b2g:master

document hidden checking removed and introduce a hasPendingInit checking that could check if the init is finished correctly. I replace the window close with navigation init again in order to init the navigation correctly, but I would prefer close the app directly because no body would expect the app is opened after removing the notification.

	Steve Chung [:steveck] Assignee
	Comment 21 • 9 years ago

Comment on attachment 8676078 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-taskrunner > mozilla-b2g:master

Task runner version of the startup checking. Not sure if it's your expected way of handling the the different startup scenario(actually there's only 2 cases: normal startup and notification startup that return early rejected init promise). However it's not working because the task queue could only be triggered as resolve in the notification system message checking.

	Julien Wajsberg [:julienw] Reporter
	Comment 22 • 9 years ago

Comment on attachment 8671193 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-fix > mozilla-b2g:master

yeah I think it's better like this, at least for v2.5. Then we can work on making the startup more solid in case of a message sequence.

Like you I think we could try to call window.close() instead of Navigation.init(). With the new "pendingInit" state you introduced, I think we are safer.

Here is especially the case we need to check:

1. receive a SMS.
2. force kill the app.
3. start the app using the icon.
4. dismiss the notification manually.
=> the app should not be closed and work correctly :)

	Julien Wajsberg [:julienw] Reporter
	Comment 23 • 9 years ago

Ah BTW I noticed the following STR has also a bad behavior:
1. receive a SMS.
2. click the notification.
=> we can see there is some time happening before we move to the right thread, even if we wait some time between 1 and 2. Maybe we shouldn't wait "onceDocumentIsVisible()" in "onNotificationClicked" ?

	Julien Wajsberg [:julienw] Reporter
	Comment 24 • 9 years ago

Comment on attachment 8676078 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-taskrunner > mozilla-b2g:master

It's not completely clear in my mind yet, let me take some more time to think about it :)

	Steve Chung [:steveck] Assignee
	Comment 25 • 9 years ago

(In reply to Julien Wajsberg [:julienw] from comment #23)
> Ah BTW I noticed the following STR has also a bad behavior:
> 1. receive a SMS.
> 2. click the notification.
> => we can see there is some time happening before we move to the right
> thread, even if we wait some time between 1 and 2. Maybe we shouldn't wait
> "onceDocumentIsVisible()" in "onNotificationClicked" ?

Yeah it might affect the launch of the notification. Removing the visibility waiting could reduce around 400ms and you can see the conversation view immediately.

I've tried the several combination of launch and notification test. The pending init case is limited only when launched with pending notification case and flag will be changed right after valid navigation.

	Steve Chung [:steveck] Assignee
	Comment 26 • 9 years ago

Comment on attachment 8671193 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-fix > mozilla-b2g:master

Hi Julien, I added the test part including a small assertion for integration test that make sure we always enter the conversation view directly after clicking on the notification. I tried to verify the app is closed after notification close but in vain. Seems like client.apps.getApp could still found the app after closing, so I'm not sure if there's any better way to verify the app is closed.

	Julien Wajsberg [:julienw] Reporter
	Comment 27 • 9 years ago

(In reply to Steve Chung [:steveck] from comment #26)
> Comment on attachment 8671193 [details]
> [gaia] steveck-chung:bug-1192263-navigated-event-fix > mozilla-b2g:master
> 
> Hi Julien, I added the test part including a small assertion for integration
> test that make sure we always enter the conversation view directly after
> clicking on the notification. I tried to verify the app is closed after
> notification close but in vain. Seems like client.apps.getApp could still
> found the app after closing, so I'm not sure if there's any better way to
> verify the app is closed.

Is it possible that we need to wait for some time until the System app finds that the process is closed ?

	Julien Wajsberg [:julienw] Reporter
	Comment 28 • 9 years ago

Here is a STR that doesn't work properly because we don't use a task runner:

1. receive 2 SMS from 2 different contacts, so we have 2 different notifications.
2. in a rapid sequence, dismiss one notification, open the other notification

=> The SMS app is not opened... because we call window.close().

	Julien Wajsberg [:julienw] Reporter
	Comment 29 • 9 years ago

Maybe a solution is to check navigator.mozHasPendingMessage('notification') and navigator.mozHasPendingMessage('sms-received') again, and not close if we have one of those ?

	Julien Wajsberg [:julienw] Reporter
	Comment 30 • 9 years ago

Comment on attachment 8671193 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-fix > mozilla-b2g:master

Some simple changes + maybe check the previous comment's case.

Thanks !

	Steve Chung [:steveck] Assignee
	Comment 31 • 9 years ago

(In reply to Julien Wajsberg [:julienw] from comment #28)
> Here is a STR that doesn't work properly because we don't use a task runner:
> 
> 1. receive 2 SMS from 2 different contacts, so we have 2 different
> notifications.
> 2. in a rapid sequence, dismiss one notification, open the other notification
> 
> => The SMS app is not opened... because we call window.close().

Sorry that I didn't raise this issue. I know this issue, but the reason I still commit the patch because:
- The notification is not always unclickable. It only happened during the app is not fully closed.
- Even the the notification clicking brings no action, the notification is still on the tray.

I'm still not sure if task runner could solve this notification event issue, because even we running the task one by one, like navigation init => notification remove => notification click. We still need some other flag or state to prevent app closing while removing the notification, otherwise the notification removal must comes earlier than clicking.

The hasPendingMessage API won't help anything here. hasPendingMessage('notification') is true means the notification system message handler function is not registered yet. So once the app is launched via system message and handler initialization is ready, hasPendingMessage would be always false no matter how many notifications pending on the tray.

I'll update the patch will another flag that help us to know is there's any other notification is triggered and entering the handling process. Please let me know if you have any other idea about this solution.

	Steve Chung [:steveck] Assignee
	Comment 32 • 9 years ago

Comment on attachment 8671193 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-fix > mozilla-b2g:master

Some comments left on github.

	Julien Wajsberg [:julienw] Reporter
	Comment 33 • 9 years ago

> hasPendingMessage('notification') is true means the notification system message handler 
> function is not registered yet. So once the app is launched via system message and 
> handler initialization is ready, hasPendingMessage would be always false no matter how 
> many notifications pending on the tray.

I wonder if this should be fixed? IMO hasPendingMessage('notification') should return true if the handler has not been called for all 'notification' messages yet.

	Julien Wajsberg [:julienw] Reporter
	Comment 34 • 9 years ago

Comment on attachment 8671193 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-fix > mozilla-b2g:master

I don't feel comfortable with latest proposal (one more state + relying on present notifications...) so I prefer to live with this very small race than adding more complex code.

If you remove the last commit it's r+ for me :)

	Steve Chung [:steveck] Assignee
	Comment 35 • 9 years ago

Comment on attachment 8671193 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-fix > mozilla-b2g:master

As you wish :)

	Julien Wajsberg [:julienw] Reporter
	Comment 36 • 9 years ago

Comment on attachment 8671193 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-fix > mozilla-b2g:master

r=me but please rebase to latest master and wait for a green try run -- last one was polluted by bug 1218211.

	Julien Wajsberg [:julienw] Reporter
	Comment 37 • 9 years ago

Also found bug 1218374 while testing but it's not provoked by this patch -- it's in master too.

	Steve Chung [:steveck] Assignee
	Comment 38 • 9 years ago

Rebased and merged in master: https://github.com/mozilla-b2g/gaia/commit/3c20223daf36255c96309c20840379929768d208
Thanks!

	Julien Wajsberg [:julienw] Reporter
	Comment 39 • 9 years ago

Comment on attachment 8676078 [details]
[gaia] steveck-chung:bug-1192263-navigated-event-taskrunner > mozilla-b2g:master

I added a last comment on this feedback request. Maybe this would be useful to rebase it on top of latest master and move this to a separate bug ?

	Julien Wajsberg [:julienw] Reporter
	Comment 40 • 9 years ago

NI so that you don't keep track of this :)

	Steve Chung [:steveck] Assignee
	Comment 41 • 9 years ago

Follow up created in Bug 1237192.