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)
Tracking
(blocking-b2g:-, b2g18+ fixed, b2g18-v1.0.1 fixed)
RESOLVED
FIXED
blocking-b2g | - |
People
(Reporter: cjones, Assigned: arthurcc)
References
Details
Attachments
(4 files)
8.91 KB,
patch
|
vingtetun
:
review+
vingtetun
:
approval-gaia-v1+
|
Details | Diff | Splinter Review |
1.68 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
944 bytes,
patch
|
vingtetun
:
review+
vingtetun
:
approval-gaia-v1+
|
Details | Diff | Splinter Review |
5.39 KB,
patch
|
timdream
:
review+
vingtetun
:
approval-gaia-v1+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
There's some fun-looking stuff in here ...
http://people.mozilla.com/~bgirard/cleopatra/#report=fd6ec7d88f4ca9a799bedaa1f3f6035bbc8d0184
Whiteboard: [profileme]
Reporter | ||
Comment 2•12 years ago
|
||
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
Reporter | ||
Comment 3•12 years ago
|
||
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.
Reporter | ||
Comment 4•12 years ago
|
||
(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.
Reporter | ||
Comment 5•12 years ago
|
||
(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.
Reporter | ||
Comment 6•12 years ago
|
||
Settings is also exhausting pmem on startup, somehow (???), which is going to make some things unhappy.
Reporter | ||
Comment 7•12 years ago
|
||
(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.
Reporter | ||
Comment 8•12 years ago
|
||
I was able to peek at this in layerscope, but I didn't record the badness.
Reporter | ||
Comment 9•12 years ago
|
||
(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 ;).
Reporter | ||
Comment 10•12 years ago
|
||
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.
Reporter | ||
Comment 11•12 years ago
|
||
Whatever is triggering this isn't the scroll inactivity timeout.
Reporter | ||
Comment 12•12 years ago
|
||
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 ...
Reporter | ||
Comment 13•12 years ago
|
||
Removing the uninit hack makes most of the badness go away, but then we get a transition into the root panel on startup :(.
Reporter | ||
Comment 14•12 years ago
|
||
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
Reporter | ||
Comment 15•12 years ago
|
||
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)
Reporter | ||
Comment 18•12 years ago
|
||
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.
Reporter | ||
Comment 19•12 years ago
|
||
(Or if you could suggest an alternate reviewer.)
Comment 20•12 years ago
|
||
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+
Reporter | ||
Comment 21•12 years ago
|
||
Reporter | ||
Comment 22•12 years ago
|
||
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)
Comment 23•12 years ago
|
||
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.
Comment 24•12 years ago
|
||
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)
Comment 25•12 years ago
|
||
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:
--- → +
Comment 26•12 years ago
|
||
FYI I also provided a patch in Bug 829066 so that the existing tests don't call Settings.init anymore.
Reporter | ||
Updated•12 years ago
|
Attachment #714395 -
Flags: review?(jones.chris.g) → review+
Comment 27•12 years ago
|
||
Comment 28•12 years ago
|
||
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+
Comment 29•12 years ago
|
||
v1-train: 701a11e6e7717a3989187721a22acdec5e57d49c
v1-train: 35db520ef216c4192f636f0fab923f1d0db9c40e
status-b2g18:
--- → fixed
Reporter | ||
Comment 30•12 years ago
|
||
Hey jhford, is this in your 1.0.1 uplift queue?
Comment 31•12 years ago
|
||
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 → ---
Comment 32•12 years ago
|
||
Attachment #715105 -
Flags: review?(21)
Attachment #715105 -
Flags: review?(21)
Attachment #715105 -
Flags: review+
Attachment #715105 -
Flags: approval-gaia-v1+
Comment 33•12 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 34•12 years ago
|
||
This should land on v1-train too, right ? We should change the status-b2g18 flag then ?
Assignee | ||
Comment 35•12 years ago
|
||
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
Assignee | ||
Comment 36•12 years ago
|
||
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?
Comment 37•12 years ago
|
||
arthur: you should probably file a new bug for that anyway.
Assignee | ||
Comment 38•12 years ago
|
||
Okay, let's use bug 842221 to track it.
Comment 39•12 years ago
|
||
To make sure the follwup get's uplifted.
Reporter | ||
Comment 40•12 years ago
|
||
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
Comment 41•12 years ago
|
||
I can reproduce.
Comment 42•12 years ago
|
||
Hm, but I can reproduce without this patch applied so.. something else probably broke it..
Comment 43•12 years ago
|
||
Nevermind. I backed this bug out wrong. I backed out the original but not the followup. Backing both out fixes this problem.
Reporter | ||
Comment 44•12 years ago
|
||
Can someone back this out please?
Reporter | ||
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 45•12 years ago
|
||
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
Reporter | ||
Comment 46•12 years ago
|
||
Are there any errors in logcat when this bug reproduces?
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → arthur.chen
Assignee | ||
Comment 47•12 years ago
|
||
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)
Comment 48•12 years ago
|
||
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)
Reporter | ||
Comment 49•12 years ago
|
||
Does your build include attachment 715105 [details] [diff] [review]?
Comment 50•12 years ago
|
||
(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 51•12 years ago
|
||
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+
Assignee | ||
Comment 52•12 years ago
|
||
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)
Assignee | ||
Comment 53•12 years ago
|
||
Comment 54•12 years ago
|
||
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+
Comment 56•12 years ago
|
||
(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)
Assignee | ||
Comment 57•12 years ago
|
||
v1-train: https://github.com/mozilla-b2g/gaia/commit/00a9a825fabea20474db3606b2521a836aa2cd2f
v1.0.1: https://github.com/mozilla-b2g/gaia/commit/b6a26c26f452d123d33d57074f9546591e0da3fd
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
status-b2g18-v1.0.1:
--- → fixed
Resolution: --- → FIXED
Comment 58•12 years ago
|
||
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)
Assignee | ||
Comment 59•12 years ago
|
||
Julien, I cherry-picked all four commits in the thread to the v1.0.1 branch. I believe your patch has been landed correctly.
Comment 60•12 years ago
|
||
that's completely true. sorry for the noise.
Updated•12 years ago
|
Attachment #714395 -
Flags: approval-gaia-v1?(21)
Comment 62•12 years ago
|
||
This doesn't go to v1-train and cause bug 844011 :/
Comment 64•12 years ago
|
||
: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 ?
Comment 65•12 years ago
|
||
Sorry I mean https://bug840322.bugzilla.mozilla.org/attachment.cgi?id=715105 is not in v1-train I think...
Keep bug 844011 is fine.
Comment 66•12 years ago
|
||
yep it seems that's true...
marking both v1.0.1 and v1-train as affected to make someone check that everything is in there.
FTR there are 4 master commits :
https://github.com/mozilla-b2g/gaia/commit/00488252b5773e1d8f6b8e4e882f37e1acf56f00
https://github.com/mozilla-b2g/gaia/commit/81630237c663e5f1647344c9383baf147d65fdbd
https://github.com/mozilla-b2g/gaia/commit/3180227ca8cd498dd6558d808320377c1f5c8c5e
https://github.com/mozilla-b2g/gaia/commit/78a227a6e4d4f91fcde6a2c67bc12a8ee78a3d26
Comment 67•12 years ago
|
||
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
Comment 69•12 years ago
|
||
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)
Comment 70•12 years ago
|
||
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)
Comment 71•12 years ago
|
||
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)
Assignee | ||
Comment 72•12 years ago
|
||
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)
Comment 73•12 years ago
|
||
(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.
Comment 74•12 years ago
|
||
(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.
Comment 75•12 years ago
|
||
(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...
Comment 76•12 years ago
|
||
(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.
Comment 77•12 years ago
|
||
(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.
Comment 78•12 years ago
|
||
3180227ca8cd498dd6558d808320377c1f5c8c5e -> eb119dc1e4157cf497465d292a140543947e578d
Updated•12 years ago
|
Comment 79•12 years ago
|
||
(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.
Description
•