Closed Bug 988782 Opened 8 years ago Closed 7 years ago

[tarako] it directly returns to Settings when backward from Keyboard Settings

Categories

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

Other
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.3T+, b2g-v1.3 unaffected, b2g-v1.3T verified, b2g-v1.4 fixed, b2g-v2.0 fixed)

VERIFIED FIXED
2.0 S1 (9may)
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3 --- unaffected
b2g-v1.3T --- verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: angelc04, Assigned: timdream)

References

Details

(Whiteboard: [sprd294279])

Attachments

(3 files, 2 obsolete files)

Attached video Steps to reproduce
* Setps to reproduce
1. Kill Settings
2. Launch Settings
3. go to Settings->Keyboards->Built-in Keyboard
4. Tap on backward button "<"
   --> A black screen and a white screen will appears first and then it directly goes back to Settings.

   [Expected] It should return to Keyboards.

Please see attached video for details.


* Build info:
Gaia d8ff994bd96c37ba9a93c343932a5441a78a0eec
Gecko 6db37b3f76b4fe2aa6f8fb5ae9e036ed99344772
BuildID 20140327060053
Version 28.1
ro.build.version.incremental=69
ro.build.date=Thu Mar 27 06:07:07 CST 2014
This does not happen on the Buri:

Gaia      812838ad0fabf51fa14435af562ddac6d26fa936
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/ba97efb0da4b
BuildID   20140326004002
Version   28.0
ro.build.version.incremental=324
ro.build.date=Thu Dec 19 14:04:55 CST 2013
Buri

This is a regression.
blocking-b2g: --- → 1.3T?
Keywords: regression
Arthur, can take a look of this?
Flags: needinfo?(arthur.chen)
do we lose all the settings that were made to the keyboard?
if the user can go back to keyboard settings by clicking again without losing any settings, this seem like a minor issue
thanks
Flags: needinfo?(pcheng)
I think the Settings app is correctly OOM'd, so this behavior is expected. Is this really a bug?
Flags: needinfo?(arthur.chen)
(In reply to Joe Cheng [:jcheng] from comment #3)
> do we lose all the settings that were made to the keyboard?
No. The settings change was saved.
> if the user can go back to keyboard settings by clicking again without
> losing any settings, this seem like a minor issue
After reproducing this issue, if user follow step 3~4 again, they can see the changes and go back to keyboards successfully.
I agree that this is a minor issue.
> thanks
Flags: needinfo?(pcheng)
minus, not blocking per comment 5
blocking-b2g: 1.3T? → -
Keywords: regression
Fabrice, can we group keyboard apps to setting apps, this bug has 100% reproduce rate.
blocking-b2g: - → 1.3T?
Flags: needinfo?(fabrice)
The built-in keyboard app is too big. We should group it into setting apps.

APPLICATION        PID      Vss      Rss      Pss      Uss  cmdline
b2g                 83   42712K   38632K   33713K   31124K  /system/b2g/b2g
Built-in Keyboa   1426   18564K   17988K   11451K    7516K  /system/b2g/plugin-container
(Preallocated a   1447   14768K   14764K    9055K    5900K  /system/b2g/plugin-container
(Nuwa)             337    3092K    3092K     987K     224K  /system/b2g/plugin-container
The keyboard settings is launched with launch(), not activity. We cannot group that.

I think we can make keyboard settings page in-process to save some memory. Fabrice, is this a good idea?
Attached file Run keyboard config frame in-inprocess (obsolete) —
Attachment #8407487 - Flags: feedback?(fabrice)
That's a minor user annoyance as it's not an action that happens often.
I'm not comfortable taking Tim's patch because it increases the risk of OOMing the parent process, which would be even worse.
blocking-b2g: 1.3T? → ---
Flags: needinfo?(fabrice)
Comment on attachment 8407487 [details]
Run keyboard config frame in-inprocess

scary!
Attachment #8407487 - Flags: feedback?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #12)
> Comment on attachment 8407487 [details]
> Run keyboard config frame in-inprocess
> 
> scary!
If we save more memory, can we land this patch.
(In reply to Fabrice Desré [:fabrice] from comment #12)
> Comment on attachment 8407487 [details]
> Run keyboard config frame in-inprocess
> 
> scary!
If we save more memory, can we land this patch?
Where will these savings come from?
(In reply to Fabrice Desré [:fabrice] from comment #15)
> Where will these savings come from?

1.modem can save about 100~200KB RAM, but there is some risk.
2.move ramdisk from boot.img to nand flash partition, save about 300KB RAM.

Can you save any memory on your side?
Flags: needinfo?(fabrice)
blocking-b2g: --- → 1.3T?
(In reply to James Zhang from comment #16)
> (In reply to Fabrice Desré [:fabrice] from comment #15)
> > Where will these savings come from?
> 
> 1.modem can save about 100~200KB RAM, but there is some risk.
> 2.move ramdisk from boot.img to nand flash partition, save about 300KB RAM.

Let's do 2. as soon as possible

> Can you save any memory on your side?

We'll get some win from bug 993282
Flags: needinfo?(fabrice)
Fabrice, the keyboard setting frame will close itself when the user leaves it. It doesn't consume more memory persistently in the b2g process.
(In reply to Fabrice Desré [:fabrice] from comment #17)
> (In reply to James Zhang from comment #16)
> > (In reply to Fabrice Desré [:fabrice] from comment #15)
> > > Where will these savings come from?
> > 
> > 1.modem can save about 100~200KB RAM, but there is some risk.
> > 2.move ramdisk from boot.img to nand flash partition, save about 300KB RAM.
> 
> Let's do 2. as soon as possible
> 
> > Can you save any memory on your side?
> 
> We'll get some win from bug 993282

We open our download tool source to you, we'll wait for you finishing download tool on Mac, then I'll submit the 300KB patch, it will add new partition and you should flash u-boot.bin.
If we can pass the test case, please group it.
It has 100% reproduce rate, so it's spreadtrum blocker MP issue.
Whiteboard: [sprd294279]
Blocks: 995119
We think the user impact will be less, should not be a blocker. thanks!
triage: not blocking, please discuss with Steven if partner is still concerned, thanks
blocking-b2g: 1.3T? → backlog
(In reply to Joe Cheng [:jcheng] from comment #22)
> triage: not blocking, please discuss with Steven if partner is still
> concerned, thanks

Our product PM is still concerned.
Jesse, can you provide patch here?
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #18)
> Fabrice, the keyboard setting frame will close itself when the user leaves
> it. It doesn't consume more memory persistently in the b2g process.

Fabrice, what's your opinion on this?
Flags: needinfo?(fabrice)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #25)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #18)
> > Fabrice, the keyboard setting frame will close itself when the user leaves
> > it. It doesn't consume more memory persistently in the b2g process.
> 
> Fabrice, what's your opinion on this?

What if the user hit "home" while in the keyboard config? At least we'll need to remove this in-process iframe when it's not active and there's memory pressure.
Flags: needinfo?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #26)
> What if the user hit "home" while in the keyboard config? At least we'll
> need to remove this in-process iframe when it's not active and there's
> memory pressure.

I can patch keyboard/settings.html to do just that.
I create a patch to make keyboard to be grouped in to current process.

may be it is not the best solutions.  because of i can not pass a parameter when it is not a MozActivity

I suggest:
1. app.launch  can pass a parameter to indicate that it is a subprocess.






NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):988782
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:
Can anyone review renfeng's patch?
Flags: needinfo?(fabrice)
Flags: needinfo?(ttsai)
Flags: needinfo?(styang)
Flags: needinfo?(kkuo)
Comment on attachment 8411663 [details] [diff] [review]
group.keyboard.from.settings.patch

Review of attachment 8411663 [details] [diff] [review]:
-----------------------------------------------------------------

This patch does not observe the memory-pressure event as asked in comment 26.
Attachment #8411663 - Flags: review-
Flags: needinfo?(fabrice)
Let me provide a fix instead.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Flags: needinfo?(ttsai)
Flags: needinfo?(styang)
Flags: needinfo?(kkuo)
I will send another patch on master branch (w/o the inproc bit of course)
Attachment #8407487 - Attachment is obsolete: true
Attachment #8411663 - Attachment is obsolete: true
Attachment #8412358 - Flags: review?(rlu)
Attachment #8412358 - Flags: feedback?(fabrice)
Comment on attachment 8412358 [details] [review]
Github: https://github.com/mozilla-b2g/gaia/pull/18661

Ideally we should the 3rd party keyboard setting to decide whether to add the keyboard app to the outOfProcessBlackList.
Attachment #8412358 - Flags: feedback?(fabrice) → feedback+
Comment on attachment 8412358 [details] [review]
Github: https://github.com/mozilla-b2g/gaia/pull/18661

Looks good to me, r+.

Thanks.
Attachment #8412358 - Flags: review?(rlu) → review+
master: https://github.com/mozilla-b2g/gaia/commit/edb4c79b740434de1bf07f34d6dad3718f9ec0f8

1.3t only accepts blockers, flagging 1.3t? first.
Status: ASSIGNED → RESOLVED
blocking-b2g: backlog → 1.3T?
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S6 (25apr)
blocking-b2g: 1.3T? → 1.3T+
Flags: needinfo?(yang.zhao)
This issue still occurs for me on today's Tarako build. The repro rate is about 4/10


1.3t Environmental Variables:
Device: Tarako 1.3t
BuildID: 20140429014002
Gaia: b5adc5a943d3abbd6ab070a47c847f2c24891cc5
Gecko: e9890f5d4709
Version: 28.1
Firmware Version: sp6821
Triage: No need in v1.4 since it is LMK issue.
Verified the issue reported in Comment 0 does not occur on latest 1.3T build following STR. Attempted 15 times without reproducing. Return to Keyboards happened each time. 


1.3T Environmental Variables:
Device: Tarako 1.3T
BuildID: 20140604014021
Gaia: 41a763154fbac34bef6baf17a201e50f52f2b72a
Gecko: fed0b4e6da6c
Version: 28.1
Firmware Version: sp6821a-gonk-4.0-5-12
User Agent: Mozilla/5.0 (Mobile; rv:28.1) Gecko/28.1 Firefox/28.1
Status: RESOLVED → VERIFIED
Depends on: 1020628
Attached file master patch
We need this master patch uplifted to v1.4 otherwise I cannot fix bug 1009368

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): See bug 1009368
[User impact] if declined: Unable to uplift and fix bug 1009368 
[Testing completed]: No know regression on master since Apr 25 (comment 36)
[Risk to taking this patch] (and alternatives if risky): Unable to uplift and fix bug 1009368
[String changes made]: None
Attachment #8435586 - Flags: review+
Attachment #8435586 - Flags: approval-gaia-v1.4?
(In reply to Ivan Tsay (:ITsay) from comment #38)
> Triage: No need in v1.4 since it is LMK issue.

This is correct for the system part only. We still need the master patch for 1.4 so the keyboard settings page manages it's life cycle correctly there.
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
Attachment #8435586 - Flags: approval-gaia-v1.4? → approval-gaia-v1.4+
You need to log in before you can comment on or make changes to this bug.