[Gaia] Mediaquery in CSS may extend load time of app

RESOLVED FIXED in Firefox 34

Status

()

defect
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gduan, Assigned: kanru)

Tracking

({perf, regression})

unspecified
mozilla35
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:-, firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [c=progress p= s= u=])

Attachments

(2 attachments)

Device: hamachi, 20140415160202 master-central build.
command: |b2gperf  --delay=10 Settings|.

| Results for Settings, cold_load_time: median:1693, mean:1700, std: 29, max:1812, min:1670, all:1777,1677,1687,1701,1713,1703,1709,1732,1692,1674,1711,1682,1686,1672,1690,1723,1700,1678,1700,1683,1695,1670,1694,1682,1685,1812,1705,1692,1674,1705

After marking the mediaquery line (only /*@media(condition) {/* and /*}*/ ) of settings_large.css

| Results for Settings, cold_load_time: median:1356, mean:1371, std: 41, max:1493, min:1336, all:1401,1363,1339,1370,1353,1346,1353,1354,1358,1352,1350,1352,1359,1472,1412,1368,1365,1370,1338,1343,1345,1336,1361,1490,1355,1354,1371,1345,1367,1493
Component: Gaia → CSS Parsing and Computation
Product: Firefox OS → Core
Please let me know if any information is not cleared enough.
Hi David,
is the result expected? Could you advise and let me know if any information is unclear ? Thanks.
Flags: needinfo?(dbaron)
Is the document initially being laid out at the wrong size (such that the media query doesn't match) and then the size changes and we have to restyle because the media query starts matching?
according to https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/style/settings_large.css

The media query statement is

```
@media (min-width: 768px) and (orientation: portrait) {
```

The original css is target for phone size device (hamachi is one of them), so the media query is not affect the rendered style.
I think this is not expected.  Does nsStyleSet::MediumFeaturesChanged ever return true during this test, and if so, why?
Flags: needinfo?(dbaron)
David, forgive I have limit knowledge about internal implement, but I could help bring more test results to clarify the issue.

Following tests test settings app with command `b2gperf --delay=10 Settings`


base on 4/28 gaia master

cold_load_time: median:1636, mean:1648, std: 40


comment out all @media in https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/style/settings_large.css (no @media)

cold_load_time: median:1290, mean:1291, std: 17


only enable one plain @media (min-width: 768px) (one @media)

cold_load_time: median:1427, mean:1431, std: 28


only one @media (min-width: 768px) and (orientation: portrait)

cold_load_time: median:1411, mean:1413, std: 30,


** multiple @media (min-width: 768px) (3 plain @media)

cold_load_time: median:1634, mean:1646, std: 50



I saw if I put 3 @media (min-width: 768px) in settings_large.css, I got the same load_time as normal one (@media, @media with portrait, @media with landscape)...
Duplicate of this bug: 1002842
Carrying over nomination from the dupe.
blocking-b2g: --- → 1.4?
Keywords: perf, regression
(In reply to Fred Lin [:gasolin] from comment #7)
> I saw if I put 3 @media (min-width: 768px) in settings_large.css, I got the
> same load_time as normal one (@media, @media with portrait, @media with
> landscape)...

The number of media queries doesn't matter; the issue is that when one (or more) dynamically changes, we have to do a lot of work.  (If more than one change, the work gets coalesced.)

I'd like to know the answer to comment 5.
Flags: needinfo?(gduan)
Flags: needinfo?(gasolin)
It's a pretty normal use case in mobile since @media will be used for different layout with portrait and landscape. (at least call twice)


Thinker, This issue increase 300ms load_time in settings.app and also impact other app that applied @media.
George and I are not familiar with gecko. Could you help find a man to check the answer of comment 5?
Flags: needinfo?(tlee)
Flags: needinfo?(gduan)
Flags: needinfo?(gasolin)
KanRu would look into this.
Flags: needinfo?(tlee)
Assignee

Comment 13

5 years ago
My test result with gecko(git) 177f1467fe65ccc8600208c17473f798118860d1 and gaia 1ba9199d69f4621b78556ab0ce716c641728dffa

* With media query, css rules
Results for Settings, cold_load_time: median:6016, mean:6072, std: 131, max:6461, min:5951, all:6277,6009,5991,5974,5966,6005,6087,6330,5951,6014,6033,6131,5980,6018,6000,5968,6059,6411,6201,6008,5998,5996,5972,6050,6044,5994,6102,6461,6116,6024

* With media query, css rules
Results for Settings, cold_load_time: median:6033, mean:6084, std: 141, max:6610, min:5947, all:6091,6125,6038,5988,6128,6091,6115,6405,5993,6027,6070,6035,5947,6020,6000,5977,6610,6218,6036,6036,6010,6030,6009,5985,6032,6019,6380,6093,6028,5991

* With media query
Results for Settings, cold_load_time: median:5480, mean:5543, std: 366, max:7417, min:5211, all:5625,5453,5443,5507,5211,5449,5339,7417,5803,5566,5518,5457,5483,5412,5500,5412,5404,5501,5596,5612,5478,5529,5452,5412,5216,5391,5420,5529,5606,5575

* empty
Results for Settings, cold_load_time: median:5160, mean:5400, std: 685, max:7562, min:4870, all:5172,5135,5162,5196,5138,5121,7496,5272,5367,5137,5136,5171,4870,5145,5137,5120,5256,7562,5367,5269,5158,5141,5146,5114,5141,5108,5164,5218,7248,5339

* With css rules
Results for Settings, cold_load_time: median:5073, mean:5289, std: 641, max:7290, min:4829, all:5112,5048,5049,5060,5052,7290,5019,4951,5215,5078,5080,4829,5062,4843,5094,5141,7079,5534,4975,5062,5052,5127,5090,5069,5032,4839,5109,7100,5559,5122
Blocking for 1.4 as this is needed by a blocker : https://bugzilla.mozilla.org/show_bug.cgi?id=1002842
blocking-b2g: 1.4? → 1.4+
comment 5?
Flags: needinfo?(kchen)
Assignee

Comment 16

5 years ago
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #15)
> comment 5?

Yes. `nsStyleSet::MediumFeaturesChanged' was called twice during start-up and all return true. It returned true because mRuleProcessors[3]->MediumFeaturesChanged(aPresContext) was true.

The first mRuleProcessors[3]->MediumFeaturesChanged(aPresContext) returned true because it created newCascade. The second call to mRuleProcessors[3]->MediumFeaturesChanged(aPresContext) returned true because it find one mCacheKey.Matches in the mRuleCascades.
Flags: needinfo?(kchen)
Assignee

Comment 17

5 years ago
If I remove all the media queries, `nsStyleSet::MediumFeaturesChanged' was still called twice during start-up, but never return true.
I set assignee per Thinker's comment 12.
Assignee: nobody → kchen
Priority: -- → P1
Whiteboard: [c=progress p= s= u=]

Updated

5 years ago
Severity: normal → blocker
Status: NEW → ASSIGNED
Whiteboard: [c=progress p= s= u=] → [c=progress p= s= u=1.4]
And which media queries changed dynamically, and why?
Is the following kind of media query affected?

<link rel="stylesheet" type="text/css" href="style/video_tablet.css" media="(min-height: 768px) and (min-width: 768px)" />
Assignee

Comment 21

5 years ago
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #19)
> And which media queries changed dynamically, and why?

http://hg.mozilla.org/mozilla-central/file/1204667a2935/dom/ipc/TabChild.cpp#l738

When TabChild receives before-first-paint, it sets the CSS viewport to a default value which triggers one MediumFeaturesChanged. It then sets CSS viewport again in HandlePossibleViewportChange which triggers again MediumFeaturesChanged.
tn: can you comment on this one? I think this code path is only reached with APZC. Can we avoid having this event fire twice at page load?
Flags: needinfo?(tnikkel)
(In reply to Kan-Ru Chen [:kanru] from comment #21)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #19)
> > And which media queries changed dynamically, and why?
> 
> http://hg.mozilla.org/mozilla-central/file/1204667a2935/dom/ipc/TabChild.
> cpp#l738
> 
> When TabChild receives before-first-paint, it sets the CSS viewport to a
> default value which triggers one MediumFeaturesChanged. It then sets CSS
> viewport again in HandlePossibleViewportChange which triggers again
> MediumFeaturesChanged.

It looks like HandlePossibleViewportChange doesn't need anything from the new FrameMetrics it gets back from the APZC, it just asks the document for the viewport info via nsContentUtils::GetViewportInfo. So can we just do the same thing for before-first-paint instead of using a default use?
Flags: needinfo?(tnikkel)
Assignee

Comment 24

5 years ago
Kats, could we remove the first SetCSSViewport (linked in comment 21)?
Assignee: kchen → nobody
Flags: needinfo?(bugmail.mozilla)
Kanru, who do you think could be a good owner for this bug? Thanks.
Flags: needinfo?(kchen)
Talked with Kanru. If we could just remove what Kanru mentioned in #24, he can own it. Otherwise, we may need to find someone to help this bug. (Maybe Kats?)
Flags: needinfo?(kchen)
(In reply to Kan-Ru Chen [:kanru] from comment #24)
> Kats, could we remove the first SetCSSViewport (linked in comment 21)?

We might be able to, but I suspect that it will break things. You're welcome to try it though, because I can't say that I can think of a specific case that will break. I just know that the code is very brittle and prone to breaking.

The main things I would test are making sure pages in the browser still load with the correct initial viewport. You should test pages with and without meta-viewport tags, as well as loading non-HTML documents (such as images) directly, and things like data URLs.
Flags: needinfo?(bugmail.mozilla)

Comment 28

5 years ago
Guys we need an owner for this, it's blocking the 1.4 release.

Kanru, looks like you're it per comment 12 and comment 27. Please go ahead with Kats' suggestions in comment 27.

Thanks,
Mike
Assignee: nobody → kchen
Any update here? What happened after removing the first SetCSSViewport call?
Flags: needinfo?(kchen)
Assignee

Comment 30

5 years ago
I found no difference when the first SetCSSViewport is removed. Pushed a try to see if it breaks anything else https://tbpl.mozilla.org/?tree=Try&rev=561293e122fa
Flags: needinfo?(kchen)
(In reply to <away until June 29> Kan-Ru Chen [:kanru] from comment #21)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #19)
> > And which media queries changed dynamically, and why?
> 
> http://hg.mozilla.org/mozilla-central/file/1204667a2935/dom/ipc/TabChild.
> cpp#l738
> 
> When TabChild receives before-first-paint, it sets the CSS viewport to a
> default value which triggers one MediumFeaturesChanged. It then sets CSS
> viewport again in HandlePossibleViewportChange which triggers again
> MediumFeaturesChanged.

I think David's asking which of these queries from the stylesheet are getting updated dynamically:
@media (min-width: 768px) and (orientation: portrait) 
@media (min-width: 768px) and (orientation: landscape)
@media (min-width: 768px)

The hamachi device tested has a 320 x 480 pixels screen size so none of these rules should ever match in any orientation.
Flags: needinfo?(gduan)
Hi Jet,
those three are for tablet (800 x 1280). We have different UI and UX spec for that device.
Flags: needinfo?(gduan)
Right. I get that these styles are for a larger device, but:

Device: hamachi, 20140415160202 master-central build.
command: |b2gperf  --delay=10 Settings|.

...indicates that the test with perf issues was run on a hamachi, which should not have matched any of those media queries. If they are matching, then we have a different issue. BTW, can you provide perf numbers using "min-device-width" and not "min-width"?
https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/settings.js#L119

...says that the Settings app is resized down from 980px, which explains the rule matching. Removing the SetCSSViewport(kDefaultViewportSize) should have fixed that. Can we get timing tests on Kanru's build?
Assignee

Comment 35

5 years ago
Sorry, I didn't write down the timing from my tests. I remember there is little or no difference before/after I remove SetCSSViewport(kDefaultViewportSize). Maybe it's because I was testing against a DEBUG build.

I'll build a normal build unagi and test it again.
(In reply to bhavana bajaj [:bajaj] [NOT reading Bugmail, needInfo please] from comment #14)
> Blocking for 1.4 as this is needed by a blocker :
> https://bugzilla.mozilla.org/show_bug.cgi?id=1002842

Looks like 1002842 is alredy fixed, which was the original reason we blocked 1.4 on this. i also spoke offline to :mike and he did not see any other reason we would be blocking this bug for 1.4, So setting this back to nomination 1.4? . Please raise any concerns now for not making this a blocker.
blocking-b2g: 1.4+ → 1.4?
Assignee

Comment 37

5 years ago
Results for Settings
cold_load_time: median:899, mean:921, std: 60, max:1102, min:872
all:930,1057,913,912,894,895,872,877,880,884,912,1061,900,896,887,
896,878,901,915,1102,915,897,885,901,880,895,916,1063,922,899

Results for Settings w/o SetCSSViewport
cold_load_time: median:761, mean:796, std: 86, max:1164, min:741
all:813,897,773,769,741,760,741,741,758,833,775,862,782,753,743,754,
749,748,756,931,802,764,762,760,760,752,1164,930,783,749

Updated

5 years ago
Severity: blocker → normal
Priority: P1 → P2
Triage: Mike, from comment 36, it looks like we don't need to block on this. Can we unnom? Kan-Ru, from comment 37, it looks like Kan-Ru has found a possible solution as well.
Flags: needinfo?(mlee)

Comment 39

5 years ago
Per Datazilla (see screenshot), the Settings app's cold load time has been drastically reduced and per @kanru's comment 37 there's no longer a significant difference in cold load time when using SetCSSViewport. Unnoming from 1.4.

Kanru,
Is SetCSSViewport all that needs to be looked at to address the originally reported issue in comment 0? If so, based on your most recent comment please resolve this was WFM.
Flags: needinfo?(mlee) → needinfo?(kchen)

Updated

5 years ago
blocking-b2g: 1.4? → ---
Whiteboard: [c=progress p= s= u=1.4] → [c=progress p= s= u=]
Assignee

Comment 40

5 years ago
(In reply to Mike Lee [:mlee] from comment #39)
> Created attachment 8449667 [details]
> Settings app Screenshot 2014-07-02 at 12.24.51 PM.png
> 
> Per Datazilla (see screenshot), the Settings app's cold load time has been
> drastically reduced and per @kanru's comment 37 there's no longer a
> significant difference in cold load time when using SetCSSViewport. Unnoming
> from 1.4.
> 
> Kanru,
> Is SetCSSViewport all that needs to be looked at to address the originally
> reported issue in comment 0? If so, based on your most recent comment please
> resolve this was WFM.

Based on the result in comment 37, remove the extra SetCSSViewport could save us 125ms in average. I think it's still worth considering.

We can prove that SetCSSViewport is always called at least once in HandlePossibleViewportChange either via RecvUpdateDimensions or before-first-paint handler so it's safe to remove the extra SetCSSViewport.
Flags: needinfo?(kchen)
Assignee

Comment 41

5 years ago
Attachment #8449946 - Flags: review?(bugmail.mozilla)
Comment on attachment 8449946 [details] [diff] [review]
Remove the extra SetCSSViewport

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

r=me assuming this has been tested sufficiently. We should keep an eye out for regressions.
Attachment #8449946 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/eba5d7aa80b0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
It seems like it is a very important fix for v2.0 from https://bugzilla.mozilla.org/show_bug.cgi?id=1035135#c7

Could you please uplift it to v2.0 ?
blocking-b2g: --- → 2.0?
Flags: needinfo?(kchen)
Assignee

Comment 46

5 years ago
Let's wait the patch be cooked a bit longer on trunk. Meanwhile, we could get the measurement to find out if the patch really helps.
Flags: needinfo?(kchen)
blocking-b2g: 2.0? → 2.0+
Whiteboard: [c=progress p= s= u=] → [c=progress p= s= u=], [NO_UPLIFT]
Backed out for causing frequent B2G layout/reftests/svg/sizing reftest failures as seen in bug 1035446, bug 1035443, bug 1034559, bug 1035675, and others.
https://hg.mozilla.org/mozilla-central/rev/0f8a2996abc9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla33 → ---
Depends on: 1035446
Depends on: 1035443
Depends on: 1034559
Depends on: 1035675
Assignee

Comment 48

5 years ago
I can't think of the connection between this patch and the test failures. The failed tests are testing that a width:auto <object> should have the width of the intrinsic width of the wrapped SVG. Unless SetCSSViewport affects that...

Pushed to try.

https://tbpl.mozilla.org/?tree=Try&rev=91a6b8bca5cd
Backing out this fix caused a 100ms regression in the Settings cold load launch time, as documented in bug 1037542. Granted, the 100ms was a gain that showed up on July 4 from this fix landing the first time. I'll continue to watch this bug, and see what happens.
(In reply to Kan-Ru Chen [:kanru] from comment #48)
> I can't think of the connection between this patch and the test failures.

They certainly went away post-backout. And your Try push shows the same type of failure it was backed out for...
Assignee

Comment 51

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #50)
> (In reply to Kan-Ru Chen [:kanru] from comment #48)
> > I can't think of the connection between this patch and the test failures.
> 
> They certainly went away post-backout. And your Try push shows the same type
> of failure it was backed out for...

Right, I also reproduced it locally with emulator..

Updated

5 years ago
Severity: normal → blocker
Priority: P2 → P1
Whiteboard: [c=progress p= s= u=], [NO_UPLIFT] → [c=progress p= s= u=2.0], [NO_UPLIFT]
That test came in with bug 294086. What are the return values for svgElem->GetViewportSize() with and without the patch?
Assignee

Comment 53

5 years ago
Currently it's very hard to reproduce it locally.. I'll try to get the value.
The problem of this bug was addressed in bug 1024893. In this case, I suggest to remove 2.0+ and also remove from QC CS list.
blocking-b2g: 2.0+ → 2.0?

Comment 55

5 years ago
(In reply to Kevin Hu [:khu] from comment #54)
> The problem of this bug was addressed in bug 1024893. In this case, I
> suggest to remove 2.0+ and also remove from QC CS list.

Tapas, any concern with doing this?
Severity: blocker → normal
Flags: needinfo?(tkundu)
Priority: P1 → P2
Whiteboard: [c=progress p= s= u=2.0], [NO_UPLIFT] → [c=progress p= s= u=], [NO_UPLIFT]
(In reply to Mike Lee [:mlee] from comment #55)
> (In reply to Kevin Hu [:khu] from comment #54)
> > The problem of this bug was addressed in bug 1024893. In this case, I
> > suggest to remove 2.0+ and also remove from QC CS list.
> 
> Tapas, any concern with doing this?

I am fine with it.
Flags: needinfo?(tkundu)

Comment 57

5 years ago
2.0- and removed from QC CS list per comment 56.
No longer blocks: CAF-v2.0-FC-metabug
blocking-b2g: 2.0? → ---
Assignee

Comment 58

5 years ago
I tried to run the tests in emulator. First I disabled reftest caching so I could run multiple times a test. For each invocation of ./mach reftest-remote --enable-oop layout/reftests/svg/sizing/reftest.list TabChild::RecvUpdateDimensions is called once at the beginning and each subsequent document reload will receive before-first-paint.

TabChild::RecvUpdateDimensions
  TabChild::HandlePossibleViewportChange()
    SetCSSViewport(800, 1000) <--- Does not really call nsIDOMWindowUtils::SetCSSViewport
before-first-paint
  SetCSSViewport(980, 480) <--- removed in patch
  TabChild::HandlePossibleViewportChange()
    SetCSSViewport(800, 1000)

The first call to SetCSSViewport has no effect because mContentDocumentIsDisplayed is false. So the only difference is that without the patch, the CSS viewport is first set to a default size then changed to the real size.

Maybe some code path is only run after view port size change?
Assignee

Comment 59

5 years ago
I'm not currently working on this.
Assignee: kchen → nobody
Flags: needinfo?(jwatt)
Flags: needinfo?(jwatt)
For middle term it's pretty important for resolving webapp load time performance with @media tag, or we'll have to do some workaround on build time (we have did that in settings). 
Due to load time impact, this issue also make future non-phone layout hardly pass the review to merge into gaia code base.


3rd-party developers will still suffer for this load time penalty with @media.

ni device team, what can we do to get this back on track?
Flags: needinfo?(ehung)

Comment 61

5 years ago
I think the best way is to nominate it as a blocker of this release.

[Blocking Requested - why for this release]: app performance issue.
blocking-b2g: --- → 2.1?
Flags: needinfo?(ehung)
Flags: needinfo?(jwatt)
jwatt:

I suspect the patch in this bug makes us not enter the code after this if():
http://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGOuterSVGFrame.cpp#359

It looks like that check may be flawed, since an uninitialized device viewport is {0,0} and matches an uninitialized SVG viewport. Should we indicate that case and force the SetViewport() call if both are uninitialized?
jet: I don't think that's the problem. As I understand it the patch changes things so that whereas we used to call SetCSSViewport twice - first with arbitrary default dimensions, then with the actual dimensions - now we only make the latter call. Post-patch, when that latter SetCSSViewport call sets the actual (non-zero) dimensions, the code that you point out should be entered since |newViewportSize| will be non-zero sized while |oldViewportSize| will be zero-sized.
Flags: needinfo?(jwatt)
I think I know what's going on here. It's not that the CSS viewport isn't being set as necessary/correctly, but rather that the improved performance is causing us to occasionally lose in a pre-existing race condition.

First, some observations about the failure mode:

Taking object--auto-auto--px-0.html as an example, this test contains an <object> that should take on the content size 70px x 0px (the size of the embedded SVG document). When this test fails it actually takes on the dimensions 300px x 150px.

This 300x150 size is the fallback size that we use in nsLayoutUtils::ComputeSizeWithIntrinsicDimensions when we can't determine a computed size based on intrinsic dimensions:

https://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?mark=4323-4323,4331-4331,4352-4352,4367-4367#4315

and also for the intrinsic dimensions of nsSubDocumentFrame:

https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp?mark=575-575,592-592#560

So it's expected that the <object> will be given computed size 300x150 if it is reflowed before the SVG document that it contains has loaded. Those dimensions should be transient though. Once the SVG document loads we should schedule a reflow of the embedding element if necessary.

As far as I can tell we currently have two mechanisms to trigger a reflow of the embedding element. First, if the 'data' attribute of an <object> changes (loading a new embedded document) we post an nsChangeHint_ReconstructFrame restyle event. Second, we have code to handle reflow of the embedding element as necessary if the intrinsic dimensions of an embedded SVG document change:

https://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGOuterSVGFrame.cpp?mark=674-675#670

We won't hit either of these cases in the SVG 'sizing' tests though, since when the nsSubDocumentFrame gets its first reflow the 'data' attribute will already have been set, and since the width/height attributes of the embedded SVG don't change we won't end up in nsSVGOuterSVGFrame::AttributeChanged either.

So basically we rely on the embedded SVG document already having had an nsSVGOuterSVGFrame by the time that the reflow of the embedding <object> element is reflowed. (We don't actually require that the nsSVGOuterSVGFrame has had a reflow, only that it exists since nsSubDocumentFrame::ComputeSize uses it to read the SVG's intrinsic dimensions.) If that is not the case then the nsSubDocumentFrame will get the 300x150 size and there's no mechanism that will subsequently resize it once the embedded SVG document finishes loading.
(In reply to Jonathan Watt [:jwatt] from comment #64)
> I think I know what's going on here. It's not that the CSS viewport isn't
> being set as necessary/correctly, but rather that the improved performance
> is causing us to occasionally lose in a pre-existing race condition.

Any thoughts on how to get around the race? If this is pre-existing, can we fix it with test timeouts?
Depends on: 1063073
(In reply to Jet Villegas (:jet) from comment #65)
> Any thoughts on how to get around the race? If this is pre-existing, can we
> fix it with test timeouts?

I'd rather not since it seems users may be more likely to encounter this pre-existing bug once the patch for this bug lands. I'd rather fix the pre-existing bug, for which I've put up a patch for review in bug 1063073.
(In reply to Jonathan Watt [:jwatt] from comment #66)
> (In reply to Jet Villegas (:jet) from comment #65)
> > Any thoughts on how to get around the race? If this is pre-existing, can we
> > fix it with test timeouts?
> 
> I'd rather not since it seems users may be more likely to encounter this
> pre-existing bug once the patch for this bug lands. I'd rather fix the
> pre-existing bug, for which I've put up a patch for review in bug 1063073.

Sweet!
Assignee

Comment 68

5 years ago
(In reply to Jet Villegas (:jet) from comment #67)
> (In reply to Jonathan Watt [:jwatt] from comment #66)
> > (In reply to Jet Villegas (:jet) from comment #65)
> > > Any thoughts on how to get around the race? If this is pre-existing, can we
> > > fix it with test timeouts?
> > 
> > I'd rather not since it seems users may be more likely to encounter this
> > pre-existing bug once the patch for this bug lands. I'd rather fix the
> > pre-existing bug, for which I've put up a patch for review in bug 1063073.
> 
> Sweet!

Cool! So this means we could land again!
Assignee: nobody → kchen
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → All
The patch for bug 1063073 should be safe to backport to branches so if you want to try and get the fix for this bug on branches that would be fine.
The patch here has also resulted if layout/reftests/svg/svg-integration/filter-html-01-extref.xhtml failing intermittently, which seems like another race. On balance I think it's better to take the patch though and work out the filter-html-01-extref.xhtml failure. I've marked the test as random for B2G and opened bug 1063987 for figuring out and fixing the issue.
https://hg.mozilla.org/mozilla-central/rev/0d30e40d7fc0
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
See Also: 961636
Duplicate of this bug: 961636
See comment 49 for the kind of start up time improvement we'd see with this if approved for 2.1.
Depends on: 1064580
Discussed in triage, and since the workaround leveled out the regression, this is pure perf win. We don't block on those, but definitely would definitely take the patch if approved for 2.1 landing by sheriffs (Fabrice, Bhavana).

Ask for approval on the patch :)

(For clarification, the difference between blocking and approval: blocking+ means we don't ship until it's fixed. approval+ on the patch means we'll take it, but if it causes problems we back it out and ship anyway.)
blocking-b2g: 2.1? → -
I'm away right now. Can someone on the B2G team pick up getting approvals, assuming you guys want that?
Flags: needinfo?(kchen)
Assignee

Comment 77

5 years ago
Comment on attachment 8449946 [details] [diff] [review]
Remove the extra SetCSSViewport

Approval Request Comment
[Feature/regressing bug #]: NA
[User impact if declined]: App/page load is slower because of unnecessary reflow. See comment 49 for the improvements.
[Describe test coverage new/current, TBPL]: Landed on m-c for a while.
[Risks and why]: Little, and the patch is easy to backout.
[String/UUID change made/needed]: NA

This should be uplifted together with bug 1063073 otherwise there will be intermittent test failures.
Attachment #8449946 - Flags: approval-mozilla-aurora?
Flags: needinfo?(kchen)
Comment on attachment 8449946 [details] [diff] [review]
Remove the extra SetCSSViewport

Bhavana - We really need a way to track B2G bugs so that bugs like this don't sit for a month and then get very close to missing their chance for uplift.

Aurora+
Attachment #8449946 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(bbajaj)
Assignee

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [c=progress p= s= u=], [NO_UPLIFT] → [c=progress p= s= u=]
(In reply to Lawrence Mandel [:lmandel] from comment #78)
> Comment on attachment 8449946 [details] [diff] [review]
> Remove the extra SetCSSViewport
> 
> Bhavana - We really need a way to track B2G bugs so that bugs like this
> don't sit for a month and then get very close to missing their chance for
> uplift.
> 
> Aurora+

sure, lets discuss offline. But, in this case it would anyways not be on release management's radar as it was not being tracked/blocked and could have ridden the trains as well.

:kanru, was this a missed uplift ?
Flags: needinfo?(bbajaj)
Assignee

Comment 81

5 years ago
(In reply to bhavana bajaj [:bajaj] from comment #80)
> (In reply to Lawrence Mandel [:lmandel] from comment #78)
> > Comment on attachment 8449946 [details] [diff] [review]
> > Remove the extra SetCSSViewport
> > 
> > Bhavana - We really need a way to track B2G bugs so that bugs like this
> > don't sit for a month and then get very close to missing their chance for
> > uplift.
> > 
> > Aurora+
> 
> sure, lets discuss offline. But, in this case it would anyways not be on
> release management's radar as it was not being tracked/blocked and could
> have ridden the trains as well.
> 
> :kanru, was this a missed uplift ?

See comment 75. I think it slipped because no one actually requested approval until comment 76. How do we mark these "nice to have" bug?
You need to log in before you can comment on or make changes to this bug.