Closed Bug 993930 Opened 10 years ago Closed 10 years ago

The Some App has very smaller scale after playing full-screen on browser(m.youtube.com)

Categories

(Core :: Graphics, defect)

30 Branch
ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla32
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: hansu9866, Assigned: botond)

References

Details

(Keywords: regression)

Attachments

(4 files)

STR:
1. run setting app
2. go to home screen with home key, and then setting app is going to background.
3. run browser app -> go to m.youtube.com -> watch full screen video.
4. switch to setting app in during playing full screen video.

problems.
setting app lost original scale(1X). it looks changed small scale

expected
doesn't change scale in other app

in additionally, 
nexus-4(v1.5) can reproduced this problems.

attachment
reproduced app's screenshot attached
Flags: needinfo?(jsmith)
Flags: needinfo?(mchang)
Who can handle this issue? Please inform some suitable person, if you know. thanks
What device was this reproduced this? What build ID?
Flags: needinfo?(jsmith)
I just tried this on a nexus 4 and can't reproduce from master. Does this happen everytime?
Flags: needinfo?(mchang)
(In reply to Jason Smith [:jsmith] from comment #3)
> What device was this reproduced this? What build ID?
Nexus4 on master(v1.5) was reproduced it.

gecko commit : 63e9fff3bca167b1ab0fce67f3ae122fe82b84ba
gaia commit : 1958454595b1fa0e061f0652ae965629993f5708
(In reply to Mason Chang [:mchang] from comment #4)
> I just tried this on a nexus 4 and can't reproduce from master. Does this
> happen everytime?

This is reproduce step.
example)run settings app -> press home button (settings app is background)->run browser app and m.youtube.com->
play any video and then fullscreen mode running -> press home button(browser app is background) -> now homescreen -> and then touch settings app(background->foreground).

example application is settings app, other application result is same for that.
Reproduced this on a 1.4 over a nexus 4 device in Taipei:

1. launch settings
2. hit home key
3. launch browser, go to m.youtube.com
4. play any video, tap the video while its playing and select full screen. Allow share fullscreen if prompted.
5. with video play, hit home key
6. hit settings icon to bring settings app to foreground.

Nom'ing blocker.
blocking-b2g: --- → 1.4?
Adding qawanted to see if we can reproduce this on a production device.
Keywords: qawanted
Attached file logcat of issue
Following repro steps at comment 7, issue does NOT reproduce on today's 1.5 master:

Device: Buri v1.5 Master MOZ
BuildID: 20140414040203
Gaia: f3abbd2d0a60f1a1618db93f8b1957cae6de379c
Gecko: 215080b813a7
Version: 31.0a1
Firmware Version: v1.2-device.cfg


Issue DOES reproduce on today's 1.4 Aurora:

Device: Buri v1.4 Aurora MOZ
BuildID: 20140414000201
Gaia: 8dff633372022723e2ebad17fe3c826436b3b258
Gecko: bc14179fc49c
Version: 30.0a2
Firmware Version: v1.2-device.cfg

Attaching a logcat reproducing the issue.
Can we check if this is reproducing on 1.3?
Keywords: qawanted
(In reply to Jason Smith [:jsmith] from comment #10)
> Can we check if this is reproducing on 1.3?

Following repro steps from comment 7, issue does NOT reproduce on Buri 1.3. Settings app didn't lose original scale when user returned to that app. 

Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140414164003
Gaia: 03340f9b00343512066ab0dceca1641ef9a5374c
Gecko: 0b0822ffdbf8
Version: 28.0
Firmware version: v1.2-device.cfg
Keywords: qawanted
In my version trace, this issue seems to be related with Bug 959694.
after apply the patch, it have normal operation.
Thanks.
QA Contact: ddixon
analysis of our member. 

This is due to issues in app bounds calculation.
This issue can be fixed by adding
width: 100%;
height: 100%;
to system/style/window.css .appwindow, .activitywindow css selector.

The patch of Bug 959694 is having this modification. 
That’s the reason why the issue is not happening.
I don't think this happens on a buri, can QA test with any device with a denser screen?
QA Wanted - Can we test this on a Flame 2.0 build?
Please QA on Flame device
I was able to reproduce this issue on a Buri device and found a regression window. 

The earliest builds we have for Mozilla Inbound start at 3/27/14 so we could not provide a deeper regression window than this one.  

Tinderbox Central 1.4 Regression Window 

Last Working Build: 

1.4 Environmental Variables:
Device: buri 1.4 MOZ
BuildID: 20140222031748
Gaia: 7c320aa3291761196ccd6ed3131faffb255f3fb6
Gecko: 2d5d5fd03050
Version: 30.0a1
Firmware Version: v1.2-device.cfg

First Broken Build: 

1.4 Environmental Variables:
Device: buri 1.4 MOZ
BuildID: 20140223084714
Gaia: 7c320aa3291761196ccd6ed3131faffb255f3fb6
Gecko: 2621a1cc8c6c
Version: 30.0a1
Firmware Version: v1.2-device.cfg

Last Working Gecko First Broken Gaia:
Issue DOES NOT reproduce here. 
Gaia: 7c320aa3291761196ccd6ed3131faffb255f3fb6
Gecko: 2d5d5fd03050

Last Working Gaia First Broken Gecko:
Issue DOES reproduce here. 
Gaia: 7c320aa3291761196ccd6ed3131faffb255f3fb6
Gecko: 2621a1cc8c6c

Gecko Pushlog: 
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2d5d5fd03050&tochange=2621a1cc8c6c
The window here is inaccurate. If you don't have inbound builds, then you should produce a window using either 1.4 or m-c.
I have rechecked this regression window using Buri M-C Tinderbox builds and have found the following results.
 
The pushlog seems to indicate a merge from mozilla-inbound to m-c, we unfortunately do not have mozilla-inbound builds available in this date range to provide a deeper window.

---

Tinderbox Regression Window:

Last Working Environmental Variables: 
Device: Buri
BuildID: 20140222031748
Gaia: 7c320aa3291761196ccd6ed3131faffb255f3fb6
Gecko: 2d5d5fd03050
Version: 30.0a1
Base Image: V1.2-device.cfg

First Broken Environmental Variables: 
Device: Buri
BuildID: 20140223084714
Gaia: 7c320aa3291761196ccd6ed3131faffb255f3fb6
Gecko: 2621a1cc8c6c
Version: 30.0a1
Base Image: V1.2-device.cfg

The Gaia ID is the same in both builds, indicating that this is a Gecko issue.

Gecko Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2d5d5fd03050&tochange=2621a1cc8c6c

---

This issue DOES still reproduce in the latest Buri Aurora build, and does NOT reproduce in the latest Buri Master build.

It appears that this was introduced in Master in February, carried on to Aurora, and then fixed in Master some time after the branch (Comment 13 suggests Bug 959694).

We can find and provide the reverse window where the issue stopped reproducing in Master if that is something that would be helpful.
Maybe graphics?
Component: Gaia::System → Graphics
Product: Firefox OS → Core
Version: unspecified → 30 Branch
Could bug 968991 be the cause of this bug?
(In reply to J Zimbrick from comment #19)
> I have rechecked this regression window using Buri M-C Tinderbox builds and
> have found the following results.
>  
> The pushlog seems to indicate a merge from mozilla-inbound to m-c, we
> unfortunately do not have mozilla-inbound builds available in this date
> range to provide a deeper window.
> 
> ---
> 
> Tinderbox Regression Window:
> 
> Last Working Environmental Variables: 
> Device: Buri
> BuildID: 20140222031748
> Gaia: 7c320aa3291761196ccd6ed3131faffb255f3fb6
> Gecko: 2d5d5fd03050
> Version: 30.0a1
> Base Image: V1.2-device.cfg
> 
> First Broken Environmental Variables: 
> Device: Buri
> BuildID: 20140223084714
> Gaia: 7c320aa3291761196ccd6ed3131faffb255f3fb6
> Gecko: 2621a1cc8c6c
> Version: 30.0a1
> Base Image: V1.2-device.cfg
> 
> The Gaia ID is the same in both builds, indicating that this is a Gecko
> issue.
> 
> Gecko Pushlog:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=2d5d5fd03050&tochange=2621a1cc8c6c
> 
> ---
> 
> This issue DOES still reproduce in the latest Buri Aurora build, and does
> NOT reproduce in the latest Buri Master build.
> 
> It appears that this was introduced in Master in February, carried on to
> Aurora, and then fixed in Master some time after the branch (Comment 13
> suggests Bug 959694).
> 
> We can find and provide the reverse window where the issue stopped
> reproducing in Master if that is something that would be helpful.

So this is reproducing inconsistently on devices. Please provide the window to help us narrow down the cause.
(In reply to Preeti Raghunath(:Preeti) from comment #22)
> So this is reproducing inconsistently on devices. Please provide the window
> to help us narrow down the cause.

Clarified in person, but that isn't what is seen here. All the above window says is that this is already fixed on master, but broken on 1.4.
Milan

Please take a look.
blocking-b2g: 1.4? → 1.4+
Flags: needinfo?(milan)
Doesn't comment 13 contain the solution?
Flags: needinfo?(milan)
Botond, could you take a quick look, see if you can reproduce the issue, see if comment 13 solves it and maybe shed some light as to where this bug belongs?
Assignee: nobody → botond
Flags: needinfo?(botond)
I can reproduce this issue with an old-ish build of master (Apr 16 Gecko, Mar 25 Gaia). However, applying the fix described in comment 13 does not fix it for me.

I will investigate further.
can not reproduce on master, but can reproduce on 1.4 pvt build.
It(In reply to Jason Smith [:jsmith] from comment #21)
> Could bug 968991 be the cause of this bug?

It looks that way.

Here's what's happening:

 - When the settings app is initially loaded, in the before-first-
   paint handler, TabChild::InitializeRootMetrics() and
   TabChild::HandlePossibleViewportChange() are both called.

    - InitializeRootMetrics() calculates an initial zoom based on
      aviewport width of 980.

    - HandlePossibleViewportChange() calculates a correct viewport
      width of 768, and calls ZoomBy() [1] to adjust the zoom by
      what is essentially the ratio of the old and new viewport
      widths. To figure out the old viewport width, it looks at
      mOldViewportWidth, sees that it's 0, and uses 980 instead,
      which matches what InitializeRootMetrics() assumed.

       - The result of this adjustment is that the zoom is as if
         it had been initially calculated based on a viewport
         width of 768. This is the correct zoom for the app.

       - Since we set the viewport of 768 that we calculate here,
         mOldViewportWidth is updated to 768.

 - When the video in the browser app goes full screen, the size of
   the settings app's iframe becomes (0, 0), and TabChild::
   RecvUpdateDimensions() is called with size = (0, 0).

 - When the settings app is brought to the foreground again,
   TabChild::RecvUpdateDimensions() is called again with a nonzero
   size. Since the size before was zero and is now nonzero, TabChild
   treates this as an "initial sizing" and calls InitializeRootMetrics()
   again in addition to HandlePossibleViewportChange().

    - InitializeRootMetrics() again calculates an initial zoom based
      on a viewport width of 980. Note however that mOldViewportWidth
      is still 768.

    - HandlePossibleViewportChange() again calculates the correct
      viewport of 768, and performs the zoom adjustment again, but
      this time the adjustment factor is 1, since mOldViewportWidth
      and the calculated viewport width are both 768. As a result,
      the zoom remains the one calculated for the 980-width viewport,
      which is incorrect and causes the reported rendering problem.

Some additional notes:

 - The reason this regression was _introduced_ by bug 968991 is that
   that's what introduced the call to InitializeRootMetrics() in
   RecvUpdateDimensions().

 - The reason this regression _went away_ in master, is that the
   video going fullscreen no longer causes the settings app's iframe
   to have size zero - instead it remains nonzero. I don't know why
   that is. If we want to know, a reverse regression window that
   points out when the regression went away would be very helpful.

I can think of three possible approaches:

 - Get TabChild to handle changes of size to zero and then back to
   nonzero gracefully. I can think of two ways to do this:

    1) Instead of using mInnerSize being zero as a proxy for having
       a valid size, have a flag that is set the first time mInnerSize
       is set. This would avoid us calling InitializeRootMetrics()
       the second time.

         --> I have tried this approach, and verified that it fixes
             the problem. Patch forthcoming.

    2) Set mOldViewportWidth to 980 in InititalizeRootMetrics().
       This would mean InitializeRootMetrics() would still be called
       a second time and set the viewport width to 980 again, but
       HandlePossibleViewportChange() would then correct this the
       same way it corrects it the first time.

  3) Consider changing the size to zero to be an invalid operation.
     Find out what change between 1.4 and master got rid of this
     invalid operation being performed (preferrably using a
     reverse regression window), and uplift it to 1.4.

Kats, any thoughts on which of these solutions is the most appropriate?

Timothy also pointed out that sending size change events at all for
apps in the background indicates that we are doing unnecessary work.
I will file a follow-up to investigate this (but not in the 1.4
timeframe, as that is not what regressed.)

[1] http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?from=TabChild.cpp#275
Flags: needinfo?(botond) → needinfo?(bugmail.mozilla)
Attached is a patch that implements approach (1) from the previous comment. I have verified that this fixes the problem.
(In reply to Botond Ballo [:botond] from comment #30)
> Timothy also pointed out that sending size change events at all for
> apps in the background indicates that we are doing unnecessary work.
> I will file a follow-up to investigate this (but not in the 1.4
> timeframe, as that is not what regressed.)

Filed bug 1001214.
Blocks: 968991
Comment on attachment 8412251 [details] [diff] [review]
Patch that implements approach (1), against 1.4

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

I think we can land this on master and uplift it to 1.4. It does make the code more robust anyway. Since the size isn't getting reset back to zero on master I'm not sure it's worth the effort to track that down.
Attachment #8412251 - Flags: review+
Flags: needinfo?(bugmail.mozilla)
Rebased patch to apply to master. Re-requesting review because I moved HasValidInnerSize() from TabChildBase to TabChild.

T-push with this patch: https://tbpl.mozilla.org/?tree=Try&rev=615afc5cac71
Attachment #8412667 - Flags: review?(bugmail.mozilla)
Attachment #8412251 - Attachment description: Patch that implements approach (1) → Patch that implements approach (1), against 1.4
QA Contact: ddixon
Comment on attachment 8412667 [details] [diff] [review]
Patch that implements approach (1), against master

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

I suspect HasValidInnerSize (and mHasValidInnerSize) need to be on TabChildBase. f? to tmeshkova.
Attachment #8412667 - Flags: feedback?(tanya.meshkova)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #35)
> I suspect HasValidInnerSize (and mHasValidInnerSize) need to be on
> TabChildBase. f? to tmeshkova.

If that's the case, then whatever code sets mInnerSize in the other derived class(es) of TabChildBase would need to be updated to set mHasValidInnerSize as well.
I think it might just be that other derived classes call HasValidInnerSize() but don't update anything. But yes, it's worth clarifying.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #37)
> I think it might just be that other derived classes call HasValidInnerSize()
> but don't update anything. But yes, it's worth clarifying.

If they don't update mInnerSize, it will always be zero, and HasValidInnerSize() will always return false, in which case why call it?
Comment on attachment 8412667 [details] [diff] [review]
Patch that implements approach (1), against master

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

I'll r+ this for now and we can change it as needed if :tmeshkova responds.
Attachment #8412667 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/d14d29f8fee6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment on attachment 8412667 [details] [diff] [review]
Patch that implements approach (1), against master

Oh, sorry for late response, what was the reason of not keeping this as part of TabChildBase?
Comment on attachment 8412667 [details] [diff] [review]
Patch that implements approach (1), against master

ok, I see it is implementation specific now.
Attachment #8412667 - Flags: feedback?(tanya.meshkova) → feedback+
You need to log in before you can comment on or make changes to this bug.