Closed Bug 840322 Opened 12 years ago Closed 12 years ago

Settings unresponsive for a bit after startup

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

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

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

People

(Reporter: cjones, Assigned: arthurcc)

References

Details

Attachments

(4 files)

STR (1) Launch settings (2) After its first paint, try to pan around There's a time after startup when (2) is laggy and unresponsive.
We're spending a significant amount of time in l10nStartup(). Does that really need to be on the critical path now that the core UI is pre-localized
The timeline basically looks like - html stuff - l10nStartup() - paint/reflow/paint/reflow/paint/... - screenshot - paint/reflow/paint/reflow/paint/... All the time is going to reflowing and painting.
(There's also a bunch of time spent initializing RIL stuff, but I think vivien already has a bug on file for that.) (In reply to Chris Jones [:cjones] [:warhammer] from comment #2) > We're spending a significant amount of time in l10nStartup(). Does that > really need to be on the critical path now that the core UI is pre-localized I just commented out l10n.js and nothing seemed to be affected.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #3) > The timeline basically looks like > > - screenshot This is happening on the critical path because settings goes idle before an IDB request finishes. Presumably this is reading the settings DB. However, there's still idle time before the response arrives, so the screenshot may not be slowing things down, just causing unresponsiveness.
Settings is also exhausting pmem on startup, somehow (???), which is going to make some things unhappy.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #5) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #3) > > The timeline basically looks like > > > > - screenshot > > This is happening on the critical path because settings goes idle before an > IDB request finishes. Presumably this is reading the settings DB. However, > there's still idle time before the response arrives, so the screenshot may > not be slowing things down, just causing unresponsiveness. I added a quick hack to prefetch the settings, and it didn't materially alter the scheduling of the screenshot. However, there's something seriously wrong with initial painting here. We're requesting a brazillion surfaces. That looks like the "real bug", though the other stuff would help.
I was able to peek at this in layerscope, but I didn't record the badness.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #4) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #2) > > We're spending a significant amount of time in l10nStartup(). Does that > > really need to be on the critical path now that the core UI is pre-localized > > I just commented out l10n.js and nothing seemed to be affected. Scratch that, it dun broke stuff ;).
So whatever is going on here is very odd. Settings is actually very responsive after startup. I can pan around as long as I want with no overhead. However, the GPS checkbox doesn't seem to load properly. If you let Settings go idle for a few seconds, then the GPS checkbox switches to a non-corrupted state ... and the app goes unresponsive for several seconds. Wherever the buffers are going, they don't seem to be going into the active layer tree because they're not showing up in layerscope.
Whatever is triggering this isn't the scroll inactivity timeout.
Looks like a settings.js transitionend handler is causing this somehow. We process some restyles, then a setInit() function runs, and then all hell breaks loose. I don't understand why we have a transition running here, or why scrolling delays it indefinitely, or why we sit idle for 1.5s after that before it runs ...
Removing the uninit hack makes most of the badness go away, but then we get a transition into the root panel on startup :(.
This gets rid of the jank on startup, and greatly speeds up "real" startup (when settings is fully interactive). The firstpaint time is basically identical. Testing with gecko21.
Assignee: nobody → jones.chris.g
Comment on attachment 713291 [details] [diff] [review] Speed up "real" startup of Settings Works identically with gecko18 and gecko21. This doesn't regress memory usage; in fact, multiple trials show it being *reduced* by 1MB or so. I suspect we were ~leaking the DOM request objects somehow.
Attachment #713291 - Flags: review?(kaze)
b? -> radar
blocking-b2g: --- → tef?
kaze, any chance to get this in your review queue? This is a relatively low-risk patch that makes a huge difference in settings responsiveness on startup. Would like to get in 1.0.1.
(Or if you could suggest an alternate reviewer.)
Comment on attachment 713291 [details] [diff] [review] Speed up "real" startup of Settings Review of attachment 713291 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/settings/js/settings.js @@ +41,5 @@ > + } > + > + // DOM isn't ready so there's nothing to update. > + if (!this._initialized) { > + return; nit: spacing
Attachment #713291 - Flags: review?(kaze) → review+
Comment on attachment 713291 [details] [diff] [review] Speed up "real" startup of Settings This is by no means a no-risk patch, but I think the risk/reward tradeoff is right here. We take several seconds off the interactive startup time for the settings app with relatively simple changes. I manually tested all the settings panels I could, and dynamic changes from outside the app like enabling/disabling wifi etc. from quick settings tray.
Attachment #713291 - Flags: approval-gaia-v1?(21)
This broke the unit tests because you moved the call to |mozSetMessageHandler| out of the protection "if (! navigator.mozSetMessageHandler)" Probably this breaks the ability that settings can run (?) in Desktop Firefox. Please fix tests before landing, either by mocking mozSetMessageHandler or adding back the "protection" in the place you call it.
fix the tests. I believe we should mock mozSettings instead of returning if it's not there. We're doing this for the about_test.js already...
Attachment #714395 - Flags: review?(jones.chris.g)
We'll hold this approval until unit tests are functioning properly. Not a blocker, but we will reconsider for uplift.
blocking-b2g: tef? → -
tracking-b2g18: --- → +
FYI I also provided a patch in Bug 829066 so that the existing tests don't call Settings.init anymore.
Attachment #714395 - Flags: review?(jones.chris.g) → review+
Blocks: 841815
please land both commit when approving.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #713291 - Flags: approval-gaia-v1?(21) → approval-gaia-v1+
v1-train: 701a11e6e7717a3989187721a22acdec5e57d49c v1-train: 35db520ef216c4192f636f0fab923f1d0db9c40e
Hey jhford, is this in your 1.0.1 uplift queue?
This patch causes a regression where the panels using input type="range" (display and sound) are not working anymore. Looking at it to see if I can come up with a fix quickly.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
This should land on v1-train too, right ? We should change the status-b2g18 flag then ?
Another regression caused by this patch comes from the misuse of this reference in the "settingChanged" function, which is all inputs binding to mozSettings failed to reflect the changes. Patch was provided in bug 842221. https://github.com/mozilla-b2g/gaia/pull/8162
I found that I got exceptions like the following while changing the setting values. E/GeckoConsole( 635): [JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "Permission denied to access property 'apps.updateStatus'" {file: "app://settings.gaiamobile.org/js/settings.js" line: 41}]' when calling method: [nsIDOMEventListener::handleEvent]" {file: "jar:file:///system/b2g/omni.ja!/components/SettingsManager.js" line: 324}] It looks like the cached setting object returned from mozSettings is read-only. Gecko throws exceptions when we try to modify the object. Could anyone confirm this?
arthur: you should probably file a new bug for that anyway.
Okay, let's use bug 842221 to track it.
To make sure the follwup get's uplifted.
This apparently caused a regression > Upon more careful inspection, I see a regression w/ this patch where tapping > "Wi-Fi" after a cold-start of Settings doesn't work. Sometimes I can get > the "Wi-Fi" item to respond after a bit of Settings app > scrolling/navigating. No solid STR yet. Can anyone reproduce? Need to fix this or back out.
Keywords: qawanted
I can reproduce.
Hm, but I can reproduce without this patch applied so.. something else probably broke it..
Nevermind. I backed this bug out wrong. I backed out the original but not the followup. Backing both out fixes this problem.
Can someone back this out please?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'm not going to be able to fix this patch up, but we should reassign this to someone else. Really need to get this fix in! :)
Assignee: jones.chris.g → nobody
Are there any errors in logcat when this bug reproduces?
Assignee: nobody → arthur.chen
Tim, this patch made the following changes: - bind 'this' to the 'settingsChanged' function. - Copy and cache the result of the request instead of the entire request object. Could you help review this? Thanks!
Attachment #715842 - Flags: review?(timdream)
With this patch, I'm still getting: E/GeckoConsole( 459): [JavaScript Error: "TypeError: this is undefined" {file: "app://settings.gaiamobile.org/js/settings.js" line: 571}] This is at this.presetPanel(panel); at the end of function lazyLoad(panel)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #49) > Does your build include attachment 715105 [details] [diff] [review]? .. apparently it has the wrong version of it. great
Comment on attachment 715842 [details] [diff] [review] patch v1 to fix wrong reference and access exceptions Is this the right bug for follow up?
Attachment #715842 - Flags: review?(timdream) → review+
Comment on attachment 715842 [details] [diff] [review] patch v1 to fix wrong reference and access exceptions 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 #): 840322 User impact if declined: The inputs binding to moz settings do not reflect the change. Testing completed: yes. Risk to taking this patch (and alternatives if risky): None String or UUID changes made by this patch: None
Attachment #715842 - Flags: approval-gaia-v1?(21)
Comment on attachment 715842 [details] [diff] [review] patch v1 to fix wrong reference and access exceptions Review of attachment 715842 [details] [diff] [review]: ----------------------------------------------------------------- Let's land that with the other patches. Thanks!
Attachment #715842 - Flags: approval-gaia-v1?(21) → approval-gaia-v1+
vivien, should all this go to v1.0.1 ?
Flags: needinfo?(21)
(In reply to Julien Wajsberg [:julienw] from comment #55) > vivien, should all this go to v1.0.1 ? v1.0.1 and v1-train.
Flags: needinfo?(21)
Comment on attachment 714395 [details] [diff] [review] patch v1 to fix tests 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 #): User impact if declined: none Testing completed: yes Risk to taking this patch (and alternatives if risky): low: merely checking if objects are there before using them, good practice, they're there on the device anyway. This is to run in Firefox and/or run the tests. String or UUID changes made by this patch: none Sorry but this patch landed in v1-train but not in v1.0.1, probably because the approval wasn't set appropriately. Fixing this now.
Attachment #714395 - Flags: approval-gaia-v1?(21)
Julien, I cherry-picked all four commits in the thread to the v1.0.1 branch. I believe your patch has been landed correctly.
that's completely true. sorry for the noise.
Attachment #714395 - Flags: approval-gaia-v1?(21)
This doesn't go to v1-train and cause bug 844011 :/
:alive> all patches in this bug have landed already. While we can backout and reopen here, I'd suggest we keep bug 844011 for the follow-up instead ?
Blocks: 844727
Just a heads up, currently only 004882... landed on the v1-train branch, none of the other 3 commits fixing regressions.
Looks like qawanted was added for the initial repro, which mwu handled in comment 41. Removing for now, but if there was still something needed after all the patch churn please re-add.
Keywords: qawanted
I am curious what to do here because of comment 67 and comment 68. Please advise on which commits are to be uplifted.
Flags: needinfo?(felash)
for me, only https://github.com/mozilla-b2g/gaia/commit/3180227ca8cd498dd6558d808320377c1f5c8c5e is missing in v1-train, but I'd really want that you check all commits (see comment 66) in all the needed branches using the |git branch --contains| command.
Flags: needinfo?(felash)
Alex, this bug is blocking-b2g-, but was completely landed to v1.0.1 after branching. Further, it was 75% landed to v1-train and landing all of this bug is a blocker to bug 843660, which is tef+. cherry-picking the last bit of this patch onto v1-train (that's already on v1.0.1!) fixes the merge conflicts we're seeing in bug 843660. What should happen here?
Flags: needinfo?(akeybl)
I provided the last patch. Based on the approval in comment 56, I landed all four patches to v1.0.1 but only my patch to v1-train because I thought all of the previous patches were already in v1-train. Let's land the missing patch from Julien (318022). (In reply to John Ford [:jhford] from comment #71) > Alex, this bug is blocking-b2g-, but was completely landed to v1.0.1 after > branching. Further, it was 75% landed to v1-train and landing all of this > bug is a blocker to bug 843660, which is tef+. > > cherry-picking the last bit of this patch onto v1-train (that's already on > v1.0.1!) fixes the merge conflicts we're seeing in bug 843660. > > What should happen here?
Flags: needinfo?(akeybl)
(In reply to Arthur Chen [:arthurcc] from comment #72) > I provided the last patch. Based on the approval in comment 56, I landed all > four patches to v1.0.1 but only my patch to v1-train because I thought all > of the previous patches were already in v1-train. Let's land the missing > patch from Julien (318022). > > (In reply to John Ford [:jhford] from comment #71) > > Alex, this bug is blocking-b2g-, but was completely landed to v1.0.1 after > > branching. Further, it was 75% landed to v1-train and landing all of this > > bug is a blocker to bug 843660, which is tef+. > > > > cherry-picking the last bit of this patch onto v1-train (that's already on > > v1.0.1!) fixes the merge conflicts we're seeing in bug 843660. > > > > What should happen here? I didn't see comment 56. We should be using flags to communicate approvals. I'll uplift this patch now.
(In reply to John Ford [:jhford] from comment #73) > (In reply to Arthur Chen [:arthurcc] from comment #72) > > I provided the last patch. Based on the approval in comment 56, I landed all > > four patches to v1.0.1 but only my patch to v1-train because I thought all > > of the previous patches were already in v1-train. Let's land the missing > > patch from Julien (318022). > > > > (In reply to John Ford [:jhford] from comment #71) > > > Alex, this bug is blocking-b2g-, but was completely landed to v1.0.1 after > > > branching. Further, it was 75% landed to v1-train and landing all of this > > > bug is a blocker to bug 843660, which is tef+. > > > > > > cherry-picking the last bit of this patch onto v1-train (that's already on > > > v1.0.1!) fixes the merge conflicts we're seeing in bug 843660. > > > > > > What should happen here? > > I didn't see comment 56. We should be using flags to communicate approvals. > > I'll uplift this patch now. Sigh, that looks like it's for an unrelated bug.
(In reply to John Ford [:jhford] from comment #73) > I didn't see comment 56. We should be using flags to communicate approvals. agreed, we should have specific approval for each branch...
(In reply to John Ford [:jhford] from comment #74) > Sigh, that looks like it's for an unrelated bug. it's not (?). for the merge commit https://github.com/mozilla-b2g/gaia/commit/3180227ca8cd498dd6558d808320377c1f5c8c5e see the PR : https://github.com/mozilla-b2g/gaia/pull/8167 and the individual commit : https://github.com/etiennesegonzac/gaia/commit/3ec9ef036bec93a3136eb5bd7d9bf13e4d98b1ff They're definitely related to this bug.
(In reply to Julien Wajsberg [:julienw] from comment #76) > (In reply to John Ford [:jhford] from comment #74) > > > Sigh, that looks like it's for an unrelated bug. > > it's not (?). > > for the merge commit > https://github.com/mozilla-b2g/gaia/commit/ > 3180227ca8cd498dd6558d808320377c1f5c8c5e > see the PR : https://github.com/mozilla-b2g/gaia/pull/8167 > and the individual commit : > https://github.com/etiennesegonzac/gaia/commit/ > 3ec9ef036bec93a3136eb5bd7d9bf13e4d98b1ff > > They're definitely related to this bug. s/bug/patch/ Given that 3180227ca8cd498dd6558d808320377c1f5c8c5e is on v1.0.1, it should either be backed out in both branches or landed in both places.
3180227ca8cd498dd6558d808320377c1f5c8c5e -> eb119dc1e4157cf497465d292a140543947e578d
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #56) > (In reply to Julien Wajsberg [:julienw] from comment #55) > > vivien, should all this go to v1.0.1 ? > > v1.0.1 and v1-train. For the record, this comment is not a valid approval to go ahead with uplift to v1.0.1 at this point. We are currently only uplifting bugs marked blocking-b2g:tef+ in order to mitigate risks of regressions while managing a shrinking burndown list of partner expectations for this release. Please flag tef? next time if this comes up in any bugs over the course of the next few days.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: