Closed Bug 858908 Opened 7 years ago Closed 7 years ago

[Buri][SMS]If received more than 2 SMS of Class 0 type, just can read one SMS detail infomation

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:leo+, b2g18 fixed, b2g18-v1.0.1 fixed)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
b2g18 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: sync-1, Assigned: etienne)

References

Details

(Whiteboard: [GCF], QARegressExclude, u=fx-os-user c=scravag-sprint p=1)

Attachments

(1 file)

AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.19.053
 Firefox os  v1.0.1
 Mozilla build ID: 20130327153857
 
 +++ This bug was initially created as a clone of Bug #431424 +++
 
 [WuYing][6915]
 
 DEFECT DESCRIPTION:
 If received more than 2 SMS of Class 0 type, just can read one SMS detail infomation
 
 
 REPRODUCING PROCEDURES:
 1.after received more than 2 SMS of Class 0 type
 2.These SMSM  will prompt in notification.
 3.pull dowm the status bar
 4.Select one Class0 SMS to tap
 5.It will show detail information
 6.But other Class0 SMS will disappear,cannot read detail meessage---KO
 
 EXPECTED BEHAVIOUR:
 read all Class0 SMS detail infomation
 
 ASSOCIATE SPECIFICATION:
 
 TEST PLAN REFERENCE:
 
 TOOLS AND PLATFORMS USED:
 
 USER IMPACT:
 medium
 REPRODUCING RATE:
 5/5
 For FT PR, Please list reference mobile's behavior:
 
 ++++++++++ end of initial bug #431424 description ++++++++++
 
 
 
 CONTACT INFO (Name,Phone number):
 
  DEFECT DESCRIPTION:
 
  REPRODUCING PROCEDURES:
 
  EXPECTED BEHAVIOUR:
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:
According the currently desktop-notification, while app launch the notifications belong it will disappear. It causes more than two class0 messages are not be read the details. Who can deal with it?
blocking-b2g: --- → tef?
Assignee: nobody → janjongboom
Identified as real problem, based on the triage meeting of this morning upping to tef+.
Oh I can't do that, then this is a message for the triage meeting for tomorrow :-)
Attached patch PatchSplinter Review
The problem lies herein:

Class 0 SMSes are handled by the SMS app. When the SMS app encounters a class 0 sms it shows a notification.
When clicking a notification the respective application will launch
All notifications for this application will be removed
If you had two class 0 SMSes, or one in notifications that you didn't read yet, it will be purged, because these SMSes don't show up in the SMS app (on purpose).
This patch introduces a way for notifications to avoid getting cleared when opening their parent application. The behaviour when clicking the notification (goes away) or 'clear all' is not affected.

So I took maybe a bit of an unusual approach, but that's because I didn't want to touch Gecko on this part (and change the API of mozNotification), probably mainly because this is a FFOS only problem so far. If you prefix the title line of the notification with a zero width space it will become a 'persistent' notification.

The reason for this is that it doesn't matter if a bug is introduced here, because the character will never be shown. In theory it could cause problems if you create people in your contacts that have a name that starts with a zero width space, but a la.
Attachment #737961 - Flags: review?(fbsc)
triage: tef+ for GCF certification blocker
blocking-b2g: tef? → tef+
Whiteboard: [GCF] [status: need review]
Attachment #737961 - Flags: review?(fbsc) → review?(francisco.jordano)
Comment on attachment 737961 [details] [diff] [review]
Patch

Fernando could you take a look to this r?

Thanks Jan, just left some feedback on the PR in github.

Great job!
Attachment #737961 - Flags: review?(francisco.jordano) → review?(fernando.campo)
Whiteboard: [GCF] [status: need review] → [GCF] [status: need review fcampo]
Comment on attachment 737961 [details] [diff] [review]
Patch

Yup, taking a first look, I don't think that modifying notifications is a good idea for a sms-specific situation. 

We should change the way 0-type sms are read instead, e.g. not deleting the sms until the alert is shown.

We can chat about it face to face to speed it up if you want :)
Attachment #737961 - Flags: review?(fernando.campo) → review-
Whiteboard: [GCF] [status: need review fcampo] → [GCF] [status: needs new patch]
Comment on attachment 737961 [details] [diff] [review]
Patch

Changed the behavior, now fires up SMS app and shows alert immediately. Note: if you're in the lock screen and a class0 comes in you won't see it until you unlock the phone.
Attachment #737961 - Flags: review- → review?(fernando.campo)
I like this fix, simpler is better IMO, and the lock screen issue is not that bad as the user is gonna see the message as soon as he unlocks the phone. Just a comment on github.
Commented in GH.
Comment on attachment 737961 [details] [diff] [review]
Patch

Cool, looks great, thanks for the quick fix, and sorry for the slow review :(

Bonus item: tt also solves the bug 846293, as it gets rid of the notifications
Attachment #737961 - Flags: review?(fernando.campo) → review+
Landed in https://github.com/mozilla-b2g/gaia/commit/444b622a5c19229517a9a813ad4d6711bb130382

Do I need to do something special because this has to go in 1.0.1 too?
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Yup, just change the flag so uplifters get notice (I just did).
Whiteboard: [GCF] [status: needs new patch] → [GCF]
Uplifted 444b622a5c19229517a9a813ad4d6711bb130382 to:
v1-train: c2e5f6c7d5017e48de8845e986d4b2d1795f6be9
v1.0.1: 6cb6aabd667ec92a542ead1f5f96e855ae9da771
Unable to test with a Buri Device. Marked as QARegressExclude.
Whiteboard: [GCF] → [GCF], QARegressExclude
(In reply to croesch from comment #15)
> Unable to test with a Buri Device. Marked as QARegressExclude.

Test on FF OS phone of  
AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.019.094
Firefox os  v1.0.1
Mozilla build ID:20130502070201.

Problems still exist.
(In reply to croesch from comment #15)
> Unable to test with a Buri Device. Marked as QARegressExclude.

I don't know why your test of this bug is ok. But If I send several class-0 messages and waited enough time when all the class-0 message arrived at the ME, only the last one could be seen. Others were flashed out by the last one.
Flags: needinfo?(croesch)
Tested this defect but the defect still exist.

Reproduced Steps:
1.Receive two messages of class-0 type.
2.The alert is displayed for the same.
3.Click on Ok button in the alert message.
4.When user clicks on alert message the 2 messages are cleared.
Expected:The second alert message should not be cleared when the Ok on the first alert message is clicked.

Please check once.

Thanks,
Leo
@Jan Jongboom [:janjongboom] 

Please check the issue once again.
Able to reproduce this defect.

STR Steps:
1.Receive two messages of class-0 type.
2.The alert is displayed for the same.
3.Click on Ok button in the alert message.
4.When user clicks on alert message of the first message the 2 message alert also cleared.
Expected:The second alert message should not be cleared when the Ok on the first alert message is clicked.

Thanks,
Leo
Flags: needinfo?(janjongboom)
Hi,

Reopening this issue.
As this fix is not working in master and in v1.

Please check.

Thanks,
Leo
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(sorry to steal from you janjongboom, we just have very near deadlines and I don't know your availability :)

Etienne - can you take a look? If not, maybe fernando.campo would be a good assignee.
Assignee: janjongboom → etienne
can we confirm if v1.0.1 is impacted?
this is reopened for master and v1 per comment 20
if only for master and v1, this should be a leo+, instead of reopening a tef+
sync-1@bugzilla.tld - does this impact v1.0.1 for you and remain a blocker? Please make this blocking-b2g:tef? if so.
blocking-b2g: tef+ → leo+
Flags: needinfo?(sync-1)
(In reply to Alex Keybl [:akeybl] from comment #23)
> sync-1@bugzilla.tld - does this impact v1.0.1 for you and remain a blocker?
> Please make this blocking-b2g:tef? if so.

I can't definitely sure whether it is a blocker and impact v1.0.1. May be it's not a blocker. But it is the basic function in the test case.
Flags: needinfo?(sync-1)
Feel free to steal :-). Anyway, if this is a bug than I guess it's a Gecko bug related to alert messages. Can anyone test:

alert(1);alert(2); 

should display two alert boxes.
Flags: needinfo?(janjongboom)
Hi Jan Jongboom,

As per my analysis this bug on alert messages need to handled in System apps in modal_dialog.js
'mozbrowsershowmodalprompt' event is getting called when we receive an alert message.
When we click on ok button of an alert message confirmHandler() function is called where we need to handle this case.

Thanks,
Leo
Whiteboard: [GCF], QARegressExclude → [GCF], QARegressExclude, u=fx-os-user c=scravag-sprint-may-20-31 p=1
Whiteboard: [GCF], QARegressExclude, u=fx-os-user c=scravag-sprint-may-20-31 p=1 → [GCF], QARegressExclude, u=fx-os-user c=scravag-sprint p=1
(In reply to Jan Jongboom [:janjongboom] from comment #25)
> Feel free to steal :-). Anyway, if this is a bug than I guess it's a Gecko
> bug related to alert messages. Can anyone test:
> 
> alert(1);alert(2); 
> 
> should display two alert boxes.

This case works, because we won't execute |alert(2)| until you clicked ok on the first alert.

It get's trickier here since the the app can actually receive the second system-message (and execute the second call to alert()) while the first alert is being displayed.

Anyway, I can reproduce the bug, I'll try something.
Blocks: 874892
The issue is being tracked on bug 874892.

I opened a new bug since it's now a system app issue and not really a follow up to this first patch.

Closing this bug.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Hi,

After sending two class-0 messages send one normal SMS,then the the normal SMS is showing in notification but the same thing is not updating in the thread_list.Once if we restart the device then only we can able to see the normal message in the thread_list.

Please check the scenario once.
Reproduced on v1-train.

Thanks,
Leo
Vivien, my understanding of how the JS execution is stopped/resumed isn't sharp enough.
Do you have any pointer on what we could do concerning Comment 29?

Thanks!
Flags: needinfo?(21)
@Etienne 

Reproduced this in 2 cases
1.Receive 2 class-0 messages for the first time,receive one normail sms.The normal SMS is not updating in thread_list.
2.After receiving class-0 messages,try sending sms from existing number.When we come back the thread_list is empty.Once restart then the device shows the existing messages

(In reply to Etienne Segonzac (:etienne) from comment #30)
> Vivien, my understanding of how the JS execution is stopped/resumed isn't
> sharp enough.
> Do you have any pointer on what we could do concerning Comment 29?
> 
> Thanks!
Looked into in a bit more:

- if the app is already launched, we receive the system message and the mozMobileMessage received event

- if the app is launched by the system message, we don't get the mozMobileMesage received event, which is used to update the UI.

So we get the app in a weird state and have to restart it in order to see the message.
(In reply to Etienne Segonzac (:etienne) from comment #32)
> Looked into in a bit more:
> 
> - if the app is already launched, we receive the system message and the
> mozMobileMessage received event
> 
> - if the app is launched by the system message, we don't get the
> mozMobileMesage received event, which is used to update the UI.
> 
> So we get the app in a weird state and have to restart it in order to see
> the message.

We discussed with Etienne IRL today. So the main issue here is that when an application enter a modal state the event handling is turned off afaict. (Let's needinfo smaug to make sure). 

Smaug, when an application makes a call to window.alert in b2g we call nsIDOMWindowUtils.enterModalStateWithWindow (at http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#293) with I think disable event handling at http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#7173 ?

Is that correct?


If yes, I would suggest to use only the system message to react on new sms/mms in order to update the list. That should resolve the bug and  make the code more consistent.
Flags: needinfo?(21)
And this time let me needinfo? smaug for real.
Flags: needinfo?(bugs)
So events from the user aren't dispatched to the window when the window is in modal state.
Flags: needinfo?(bugs)
Thus maybe we should reopen this bug for all the above problems, or at least fire a new bug for this serious of problems.
(In reply to 田旻 from comment #36)
> Thus maybe we should reopen this bug for all the above problems, or at least
> fire a new bug for this serious of problems.

Since the description and STR are different I think a new bug would make sens.
(In reply to Etienne Segonzac (:etienne) from comment #37)
> (In reply to 田旻 from comment #36)
> > Thus maybe we should reopen this bug for all the above problems, or at least
> > fire a new bug for this serious of problems.
> 
> Since the description and STR are different I think a new bug would make
> sens.

I have filed a new bug.
Bugzilla ID: 876614.Please check it
Flags: in-moztrap-
Flags: needinfo?(croesch)
You need to log in before you can comment on or make changes to this bug.