STR: Open email app, open a HTML email containing external images do full zoom in by pinch method Expected: All parts of the image are visible Actual: Right part of the image is clipped, invisible. Rest of the image is fuzzy as well Version Info: Gaia 4d1d0ea5a82cddeeab497774cfa1703639e3c7d9 Gecko https://hg.mozilla.org/mozilla-central/rev/dc352a7bf234 BuildID 20140826040204 Version 34.0a1 ro.build.version.incremental=110 ro.build.date=Fri Jun 27 15:57:58 CST 2014

[Blocking Requested - why for this release]: Zooming in on email is broken

Assuming this is a regression?

Zooming in email is handled by CSS scaling in email, not APZ, so it might be a gaia-side problem. Might also be platform-side, but we shouldn't conclude one way or the other without investigation first. A regression window would help, as would a copy of the HTML email you're using to test.

Agree with :kats. One of the fallouts of the pinchy-zoomy bug for email was that we did expect to eventually see tiling glitches, but there also may be situations where we screw up resizing our iframes and such. I'd like to make sure this is a graphics problem and not an email app problem. :njpark, can you provide a copy of the email using the instructions at https://wiki.mozilla.org/Gaia/Email/ProvidingEmailsForDebugging? Thanks!

(In reply to Andrew Sutherland [:asuth] from comment #4) > Agree with :kats. One of the fallouts of the pinchy-zoomy bug for email was > that we did expect to eventually see tiling glitches, but there also may be > situations where we screw up resizing our iframes and such. > > I'd like to make sure this is a graphics problem and not an email app > problem. :njpark, can you provide a copy of the email using the > instructions at > https://wiki.mozilla.org/Gaia/Email/ProvidingEmailsForDebugging? Thanks! Hi :asuth, actually you can use this attachment for another bug, since it also exhibits the clipping: https://bugzilla.mozilla.org/attachment.cgi?id=8483726

Switching to qawanted to do branch checks first.

This issue occurs on 2.2 Flame, 2.1 Flame, and 2.0 Flame. Environment Variables: Device: Flame 2.2 Master BuildID: 20140912061053 Gaia: b72909030e214175144342f7e5df7e88a2b52fd4 Gecko: 59d4326311e0 Version: 35.0a1 (2.2 Master) Firmware: v165 User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0 Device: Flame 2.1 BuildID: 20140912123307 Gaia: d4de4123c678b198692af256254a26507e9c9a08 Gecko: 0807d8ab3e5a Version: 34.0a2 (2.1) Firmware: v165 User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0 Device: Flame 2.0 BuildID: 20140912111207 Gaia: 5a4939dc272cd1cdfa070f1df24f4787c2ae3db5 Gecko: 45dbab3b5a16 Version: 32.0 (2.0) Firmware: v165 User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0 Result: Portions of the images cannot be seen after zooming in. ----------------------------------------------------------------------- This issue does not occur on 2.2 Open_C or 1.4 Flame. Environment Variables: Device: Open_C 2.2 Master BuildID: 20140912061053 Gaia: b72909030e214175144342f7e5df7e88a2b52fd4 Gecko: 59d4326311e0 Version: 35.0a1 (2.2 Master) Firmware: P821A10v1.0.0B06_LOG_DL User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0 Device: Flame 1.4 (Base) BuildID: 20140814202332 Firmware: v165 User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0 Result: After zooming in the full image can still be seen.

Returning Regressionwindow-wanted tag note to QA-Contact / window worker: We most likely do not have the KK based builds necessary to get a window so once that has been ruled out please verify repro on JB and then use those builds for the window if necessary.

[Blocking Requested - why for this release]: swapping 2.1+ to 2.0? as this was nominated before affected branches were identified.

Triage reviewed, and decided to block because this is a regression. Pinch and zoom is no longer functional in emails per the screenshot, as you can't really read/view what's there. 2.0 is way over-ripe at this point, so any change has a high risk associated with it. Do you know what the fix is? Is there a back-out option?

(In reply to Dietrich Ayala (:dietrich) from comment #10) > ... > Do you know what the fix is? Is there a back-out option? We should be able to get closer to answering that with the regression window. No-Jun - this happens with all (external) images and all the time? Or is there a particular size/number/other apps open that affects the result?

(In reply to Milan Sreckovic [:milan] from comment #11) > (In reply to Dietrich Ayala (:dietrich) from comment #10) > > ... > > Do you know what the fix is? Is there a back-out option? > > We should be able to get closer to answering that with the regression window. > > No-Jun - this happens with all (external) images and all the time? Or is > there a particular size/number/other apps open that affects the result? This does not happen with all images. I think it is more likely to occur if the embedded image in the email is large size, like https://bugzilla.mozilla.org/attachment.cgi?id=8483726. It happens almost always with the image size this big.

== Analysis So, per comment 5's indication of the other bug's mail I now recognize this testing mail (which I already have). I strongly suspect this is just the flip-side of bug 1058831 (also 2.0+ blocker) where instead of displaying things correctly at the cost of having memory-spikes that potentially result in us getting OOM-killed, we instead have a bunch of graphical corruption but don't use up memory. *Note that you will get one behaviour or the other, so if you are seeing memory spikes, you won't see this bug*. You may need to restart the email process or the entire b2g tree to see a different behaviour. The only thing you can back out is bug 1019693, but that will just result in us always getting OOM-killed. == Conclusions It doesn't look like we're any closer to addressing bug 1058831 or even figuring out what 2.1 patch would improve things on 2.0. And I agree with Dietrich that 2.0 is likely sufficiently risk-averse at this point that the bug 1058831 OOM issue is unlikely to be able to accept an uplift, and we definitely are nowhere near dealing with this corruption. So, I suggest we just go with a migitation on v2.0 and entirely disable pinch/zoom for HTML emails that trigger newsletter mode and always show them at 100%. It sucks, but if we have to choose a single zoom level to render at, arguably 100% is the right thing to do. And in fact Dietrich's saying "as you can't really read/view what's there" implies 100% is the right choice as well. This will also address bug 1058831. We will want to revisit this bug as a 2.1+ blocker since our drift is low enough that we might be able to keep pinch/zoom. I am very open to other suggestions. I will discuss this with the rest of the Productivity team tonight at our weekly meeting and will do a little more research before that meeting. == Considerations for Alternatives Note that we previously did used to just have the HTML iframe pan inside its own fixed-size clipping window. This was a UX nightmare and triggered many APZ edge-cases that hopefully no longer exist, but I don't really want to find out. (APZ in fact recently had such a regression on v2.1, tracked as email bug 1063792 and potentially fixed on v2.1 by bug 1062437. v2.0 is allegedly unaffected but it's possible that email just doesn't have the bug on v2.0 that would manifest the problem, etc. etc.) So I suggest we don't go down that road. We don't experience memory peaks at initial HTML display time (and external images display time, at least where we don't update the scale). It seems like only dynamic manipulation of the transform causes us headaches. So if we were to change the email app to re-create the HTML document from scratch for each zoom we might be able to mitigate. The flip-side is the synchronous reflows take a long time on hamachi-class devices, so that could very well suck too much to be useful. I'll leave the needinfo against me for feedback from the meeting and my research.

At the FxOS productivity meeting we discussed this (both the corruption and OOM-killings of the ying-yang bug 1058831). :sri is taking the action item to discuss with product and friends as to whether we want to go straight to a mitigation or consider other low-risk alternatives, including the possibility of just seeing if this turns out to be a real problem on actual 2.0 devices or not and then applying the bad-for-UX mitigation at that point.

Sri asked for some additional mitigation options. So I experimented with a whole bunch of stuff. In the process I discovered https://bugzilla.mozilla.org/show_bug.cgi?id=1058831#c41 which seems to imply that the "too much memory" problem may be related to some type of timing race. Unfortunately, that still appears to be orthogonal to this bug's happens-sometimes graphical corruption problems. Good news: - I found a sure-fire mitigation for both memory spikes and graphical corruption by having us detach the iframe and reattach it. - I found a much improved UX experience by using quantized zoom levels (fit-width, 100%, 150%, 200%) where you need to do a pinch gesture to move up/down those quantized zoom levels. For zooming in we wait until our absolute scale crosses 1.15 to zoom in or 0.9 to zoom out. Especially if we're just modifying "transform: scale(blah)" this performs way better than what we're using now in terms of visual feedback and memory usage. Unfortunately, we can't escape probabilistically generating memory spikes (as noted on bug 1058831 comment 41), and graphical corruption is still a thing. We also do get some visual flicker while racing the compositor thread or whomever to fix-up our scroll positions. Bad news: - The sure-fire mitigation takes about 3 seconds to actually result in visual feedback and jumps our memory usage up a fair bit in steady state and results in the production of non-trivial amounts of garbage when zooming that can take some time to be collected. - Sometimes after zooming our scrolling seems to break. It's not clear to me if this is an APZ v2.0 thing or maybe just the gesture_detector.js logic maybe interfering with APZ scrolling, possibly because we're freaking out the gesture_detector's state machine by transforming things on the fly. Summary: I think there's some promising stuff in my mitigations, but I think our only realistic option is to do the "newsletter mode is locked to non-pinch-zoom 100%" as things stand.

side-loadable mitigation experiment zip. You unzip it to a directory, add the directory as a project in the WebIDE, then hit play to install it. Or something like that. The defaults in the experiment zip are to use the quantized pinch-zoom with actual transform scaling. To mess with settings, debug the app and, after displaying at least one HTML email (to trigger the dynamic require()), the control settings for the iframe_shims.js file will be present at window.iframeShimOpts. Using the JS console attached to the process you can turn on things like the slow-but-reliable workaround like so: window.iframeShimOpts.reparentOnResize = 'iframe'; or turn it off again window.iframeShimOpts.reparentOnResize = null; If you want to see memory spikes, crank up the zoomDelayMS to something like 3000 and then do two or more separate pinch zoom-in gestures before the timer fires. You should experience a memory spike when the timeout fires and we trigger the zoom from within the timeout handler: window.iframeShimOpts.zoomDelayMS = 3000; A pull request is up at https://github.com/mozilla-b2g/gaia/pull/24137 with the contents of the branch. There's 2 checkpoint states in there. In particular, falling back to the second commit will lose the quantized zooming and instead go back to continuous zooming but with various zoom stabilization and zoom throttling logic at play.

FWIW, i can reproduce the crash (bug 1058831) and memory spike with HTML emails when pinch zooming on a ZTE Open 2 (256Mb RAM) device. This confirms that it is not specific to the Flame at low configuration only, and will reproduce at other low-mem devices.

(In reply to Tony Chung [:tchung] from comment #17) > FWIW, i can reproduce the crash (bug 1058831) and memory spike with HTML > emails when pinch zooming on a ZTE Open 2 (256Mb RAM) device. This > confirms that it is not specific to the Flame at low configuration only, and > will reproduce at other low-mem devices. And for completeness, this was on a 2.0 build. Gaia 31434a3949556171f3565ca47ac2b44e810e95e6 Gecko https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/989a723d7e2e BuildID 20140917000200 Version 32.0 Device Name ZTE_P821A20 FW-Release 4.3 FW-Incremental eng.chencong.20140715.162404 FW-Date Tue Jul 15 16:25:44 CST 2014

Mozilla-Inbound Regression Window: Last working: Device: Flame 2.1 BuildID: 20140812163115 Gaia: 9f35fca9d818b26c06aa6b7e5c0bef25886f8f20 Gecko: bcdb93a50b8e Version: 34.0a1 (2.1) Firmware: V123 User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0 First Broken: Device: Flame 2.1 BuildID: 20140812171214 Gaia: 9f35fca9d818b26c06aa6b7e5c0bef25886f8f20 Gecko: 046dc36c93b1 Version: 34.0a1 (2.1) Firmware: V123 User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0 Last working Gaia First Broken Gecko: Issue DOES reproduce Gaia: 9f35fca9d818b26c06aa6b7e5c0bef25886f8f20 Gecko: 046dc36c93b1 First Broken Gaia Last working Gecko: Issue does NOT reproduce Gaia: 9f35fca9d818b26c06aa6b7e5c0bef25886f8f20 Gecko: bcdb93a50b8e Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=bcdb93a50b8e&tochange=046dc36c93b1 Caused by Bug 1019693

Caused by Bug 1019693 - can you take a look Matt?

Please keep in mind that bug 1019693 was an improvement bug with the expected outcome of this bug and bug 1058831 coming into existence. Bug 1019693 is not meaningfully a regression since we were addressing a significantly worse problem and making it less bad. Just like for bug 1058831 the only way to perform a meaningful regression is to bisect using git, applying the patch for bug 1019693 at each point.

This installable zip has been further altered to start the initial zoom at 100% but uses quantized pinch-zoom to start at 100%. From there the user can pinch to zoom out to the fit-to-width scale or zoom in to 150% and then to 200%. Setting needinfo for people to evaluate: - The initial 100% experience. If you don't pinch/zoom, this is what the user would experience. This is our surefire mitigation fix. - The quantized pinch/zoom with suppression in place. Tony, if you can try this on a ZTE Open 2 again or other realistic 2.0 device, that would be great. Note that I am interpreting your previous realistic 2.0 device testing to have been with the existing code which operates significantly differently from the quantized zoom. (The old way was almost certain to generate a memory spike or graphical corruption with a single zoom gesture, and if that gesture oscillated between pinching in and out it could get real bad.)

I tried it out on 2.0 Flame just for the sake of it, and personally I preferred this much more than the existing email app, since most people don't need in-between resolution- either the fit to screen size or 100% view is sufficient in most cases. (Although the graphical transition would make it look cool, but it doesn't add much functionality I think). Also there was no issue of app dying when I oscillate between different zoom resolution. And it was fast.

Thanks for testing! If we all agree that fit-to-width and 100% are sufficient and higher zoom levels aren't needed, that would probably also let us control the memory spikes if/when they happen. Specifically, I believe I found in my other research that higher zoom levels do correspond to higher memory spikes (since there are more tiles to render when we end up rendering the iframe in its entirety.) I think the main argument for having a higher zoom level is cases where font directives aren't working out right. I haven't actually seen this in practice, however, and it seems like "font inflation" logic doesn't come into play for us which is where I would expect the most wackiness. (a la https://developer.mozilla.org/en-US/docs/Web/CSS/text-size-adjust)

This feels very usable to me. Maybe just because it's taking larger jumps, it actually feels more responsive/correct to me than the current app on master, which usually zooms one more level after I've stopped the pinch/zoom gesture and gives an impression of lag. With the quantized approach it just makes a single jump in response to the gesture and I can get on with my email. > If we all agree that fit-to-width and 100% are sufficient and higher zoom levels aren't needed I'd prefer this current patch to having just 100%/fit-to-width levels but it all depends on how successful this is at eliminating/minimizing the occurrence of the bug.

(In reply to Andrew Sutherland [:asuth] from comment #24) > Thanks for testing! If we all agree that fit-to-width and 100% are > sufficient and higher zoom levels aren't needed, that would probably also > let us control the memory spikes if/when they happen. Specifically, I > believe I found in my other research that higher zoom levels do correspond > to higher memory spikes (since there are more tiles to render when we end up > rendering the iframe in its entirety.) > > I think the main argument for having a higher zoom level is cases where font > directives aren't working out right. I haven't actually seen this in > practice, however, and it seems like "font inflation" logic doesn't come > into play for us which is where I would expect the most wackiness. (a la > https://developer.mozilla.org/en-US/docs/Web/CSS/text-size-adjust) I got the patch running on an Open 2, running 2.0. same gaia/gecko build as my comment 18. Observations: * crashes and memory spikes dont happen anymore. * pinch/zoom or double tapping a email will cause graphical corruption. no trickiness Screencast and screenshot included: http://youtu.be/ZusSz-EwGOA

(In reply to Tony Chung [:tchung] from comment #28) > Created attachment 8492255 [details] > screenshot of quantized zoom corruption commented in IRC, but in my opinion we need to consider the risks to both scenarios before we move forward with a plan. Option 1: sticking to current app on 2.0 Pros: can pinch zoom and pan to see the full content of the email. Everything draws properly after loading Cons: memory spike and instability (email app will crash) occur when you do this on a low memory device Option 2: taking the quantized zoom patch on 2.0 Pros: memory is stable and no crashes when pinch zooming and panning of the email Cons: graphical image corruption. Content never draws after rightside tiling. If the user needs to see and click the data on the right side, they can't do it in the zoomed mode, only the 100% scaled view. Both current options have good pros and cons. I do agree that we cant be crashing and lagging performance of the app on these lower memory devices. (and its easy to reproduce). But i also believe we can't be loading missing content in an email that is unviewable to the user. Thoughts? what is matt's progress so far on this for 2.0?

So I think this addresses the graphical corruption quite solidly. The key thing I did was to make it so that we never have any horizontal scrolling inside the iframe itself. This seemed to be the trigger for both :tchung and :doliver's repro cases, resulting in the "remainder" tiles (rightmost, bottommost) being corrupted on v2.0 and not painted on v2.2/trunk. We accomplish this by having improved sizing heuristics to avoid having horizontal scrolling inside the iframe. I've set the initial display level back to page-width because things seem so great. I'm not seeing any memory spikes but we can still reproduce them if we hit the setTimeout scenario.

Comment on attachment 8492403 [details] perfect, flawless, quantized zoom initially set to page-width patch is much better. it doesnt affect the memory spike or instability of crashes; and also loads and draws the full page as expected when doing both pinch zoom and double tap. QA-approval+

This is just a cherry-pick of the 2.0 branch. I'm doing the formal one against trunk so that the test results (which should be fine) are more meaningful. This consists of: - The cleanup changes you requested / which were prudent anyways - A pretty thorough rewrite of the big doc-block - I put back in the 'noninteractive' bail logic because otherwise compose mode would freak out. (compose uses iframes to display replied-to/forwarded HTML content as an immutable iframe hunk.) I had removed it previously because we originally would not attach the pinch-zoom logic in !newsletterMode, however that was too much. We do not want pinch-zoom in compose mode for many reasons, most of all that it doesn't work right. I'm not providing new zips, but if someone wants them, I can. Or just pull this trunk branch or the v2.0 one that's at https://github.com/mozilla-b2g/gaia/pull/24137

Comment on attachment 8492502 [details] gaia/master pull request Tested and reviewed the 2.0 patch. The zoom seems solid, and tried the compose view from a reply. Left some comments that were just editorial, the only thing that would be nice to see get in is the comment about the DEFAULT_STYLE_TAG addition for body > blockquote for replies in compose.

Ran into an integration test snafu where a buggy browser test kept implying the email app was broken when it was not in a sufficient statistically relevant way that it couldn't be ignored. :jrburke and I investigated it and verified it wasn't email and I have a guess at what I think the race is in the st; I've filed bug 1071349 on that and we'll try to help see that to fruition. In the meantime, this is landed on trunk: landed on gaia/master: https://github.com/mozilla-b2g/gaia/pull/24238 https://github.com/mozilla-b2g/gaia/commit/c6f5c85a7d36016b64be0e40c850601a55234ab0 The patch cherry-picks cleanly to v2.0 and I assume v2.1 as well. Requesting approvals now.

I am moving this bug back to the e-mail app because this entire fix is a mitigation fix implemented in the e-mail app. There continues to exist graphical corruption bugs in the platform and accordingly I will spin off a new bug for that in Core::Graphics, ideally with a simple example test file.

Comment on attachment 8492502 [details] gaia/master pull request [Approval Request Comment] [Bug caused by]: This is not meaningfully a regression. Ignore the regression windows. Pinch/zoom has been triggering OOM-death and corruption problems since APZ got turned on and possibly before. There is nothing you can back out. [User impact] if declined: Email pinch/zoom will continue to be unresponsive and result in OOM-death and visible graphical corruption. [Testing completed]: Extensive testing on v2.0 and v2.2 devices by :asuth, :jrburke, :tchung (who granted qa-approval+), :njpark, and :doliver. [Risk to taking this patch] (and alternatives if risky): This is a mitigation fix that drastically reduces the incidence of memory-spikes/OOM-death making it almost impossible to even trigger memory spikes. It also massively improves usability through the change to using quantized pinch-zoom (we jump directly from fit-to-width to 100% rather than trying to smoothly interpolate as the user moves their fingers, something the platform is unable to keep up with in realtime). This is a ridiculously awesome improvement to what we had before. Our only remaining alternative is to disable pinch-zoom entirely with HTML mails always shown at 100%. [String changes made]: No string changes.

v2.1: https://github.com/mozilla-b2g/gaia/commit/3b58e386b8c56d6080d966dcabb1c758741f3584 v2.0: https://github.com/mozilla-b2g/gaia/commit/263e3b201dca967ec5346e35901aa981ca47dce7

No-jun, please check that 2.0 and 2.1 is fixed tomorrow. thanks!

Verified in 2.0 and 2.1. the app no longer crashes in normal use case scenarios, and it is much more responsive than before.