arni2033 Reporter
	Description • 9 years ago

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20151221030239
STR:
1. Open https://translate.google.com/
2. (Quickly?) press Ctrl+Shift+M to enter/exit Responsive Design Mode several times

Result:       All images on the page blink when I enter/exit Responsive Design Mode
Expectations: Images should not blink

Regressed between 2015-10-30 and 2015-10-31 by bug 1207355
> pushlog_url:   https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=99a4fb4ba5c1ee96ac745c6ad11894bf0537f73a&tochange=2e19045ba652ca2a5a5fc0e20d6f95293acfa32d

	Daniel Holbert [:dholbert]
	Comment 1 • 9 years ago

I can reproduce this fairly reliably, using current Nightly. (I see this happen with the "Google" word-logo in the upper-left, as well as the watering can towards the bottom of the left.

Seth, mind taking a look?

	Jet Villegas (inactive)
	Comment 2 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #1)
> I can reproduce this fairly reliably, using current Nightly. (I see this
> happen with the "Google" word-logo in the upper-left, as well as the
> watering can towards the bottom of the left.

I was able to repro this one in FF45 but not in Nightly, likely because of the fix for bug 1225934.

	arni2033 Reporter
	Comment 3 • 8 years ago

Reproducible on:   Win7_64, Nightly 48, 32bit, ID 20160313030418

	Daniel Holbert [:dholbert]
	Comment 4 • 8 years ago

Like arni, I can still reproduce this in Nightly, though it it subjectively seems like the flicker-pause has gotten shorter. (not sure)

(I tested using 48.0a1 (2016-03-12), which is well after bug 1225934 landed. So, I suspect bug 1225934 didn't fix this.)

	Doug Turner (:dougt)
	Comment 5 • 8 years ago

can we fix or back out asap?

	Daniel Holbert [:dholbert]
	Comment 6 • 8 years ago

I don't think a backout would make sense at this point.  The only known STR involve a pretty niche use-case [rapidly switching into/out of responsive mode], and in current Nightly, the symptom is very brief & subtle -- it's much less noticeable than in the attached screencast.  And backing out would involve a good deal of risk, since the regressing bug, bug 1207355, seems to have been decently complex, and landed ~5 months ago, and has had several followups land afterwards that depend on it -- e.g. bug 1151359, bug 1225934.

So, cost/benefit of a backout doesn't make sense to me.

I agree we should investigate & fix this ASAP, though. Needs attention from either seth or tn, who were involved on the regressing bug.

	Daniel Holbert [:dholbert]
	Comment 7 • 8 years ago

For the record, here's a screencast on YouTube comparing the STR on Firefox 44 (left, unaffected by this bug) to Firefox 45 (center, affected) & current Firefox Nightly (right, affected).

(As shown in that screencast: instead of exhibiting this bug [with just the images blinking], Firefox 44 seems to exhibit something a bit worse -- the full viewport blinking blank instead.  Maybe this is because our full-page paint was blocked on the image being synchronously decoded, or something like that? Not sure.)

	Daniel Holbert [:dholbert]
	Comment 8 • 8 years ago

Screencast link: https://www.youtube.com/watch?v=1tKDOPiHRpw

	Timothy Nikkel (:tnikkel)
	Comment 9 • 8 years ago

The affected images have Discard called on them because the presentation for the content document is destroyed. The presentation is destroyed when we leave design mode because of this code (which is called via restoreScrollbars)

http://mxr.mozilla.org/mozilla-central/source/devtools/client/responsivedesign/responsivedesign-child.js?rev=a88b9e9d747a#142

which comes from bug 1067145 and doesn't really have a clear explanation that I can find for why it's needed. The code (and the comment) seem explicitly crafted to destroy the presentation. Since all the calls to flushStyle happen after adding or removing a style sheet, it seems the point is to ensure that stylesheet add/remove takes effect. But recreating an entire presentation seems overkill for that, is that actually needed?

	Jet Villegas (inactive)
	Comment 10 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #6)
> I don't think a backout would make sense at this point.  The only known STR
> involve a pretty niche use-case [rapidly switching into/out of responsive
> mode]

Let's not back out over this one. I'm curious about why responsive mode requires that it tear down the entire presentation per comment 9.

	arni2033 Reporter
	Comment 11 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #6)
> The only known STR involve a pretty niche use-case [rapidly switching into/out of responsive mode]
Clearly, that's wrong. It happens every time, no matter how "rapid" I press Ctrl+Shift+M. See Summary

> and in current Nightly, the symptom is very brief & subtle --
> it's much less noticeable than in the attached screencast.
Huh, it just looks like you're comparing your PC vs my PC... On Nightly 2016-03-18 (your comment) the synptom is the same as it was on "first bad" build [2]. On your screencast they look ~the same too.

(In reply to Daniel Holbert [:dholbert] from comment #7)
> (As shown in that screencast: instead of exhibiting this bug, Firefox 44 seems to exhibit
> something a bit worse -- the full viewport blinking blank instead.
I haven't seen that bug on "last good" and "first good" builds. Is it because you tested non-e10s?

Though, I always saw some lags when I hold Ctrl+Shift+M, e.g.:
"When I focus urlbar and hold Ctrl+Shift+M for 5-10 seconds, Firefox hangs consuming 1.5 Gb Ram".
Those lags slightly improved after bug 1207355 in case when web page is focused, but they are still
presented on "first bad" build. It's probably a memory leak, but bug 1256315 prevents me from testing

Screencasts:
(in each testing I first switch mode relatively slow, then - very fast)
> [1] (Blinking) "last good" vs "first bad"    https://dl.dropboxusercontent.com/s/37apm9uuk9p49tf/bug%201234276%20comment%2011%201.webm?dl=0
> [2] (Blinking) "first bad" vs "2016-03-18"   https://dl.dropboxusercontent.com/s/nm68olqsuls4uwn/bug%201234276%20comment%2011%202.webm?dl=0

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 12 • 8 years ago

While this is an edge case, it also sounds like a definite regression and we have a possible cause. 
Paul, can you take on this bug?

	Timothy Nikkel (:tnikkel)
	Comment 13 • 8 years ago

Although what devtools is doing here is a little wierd, imagelib used to handle it fine, and it seems reasonable that it should also continue to handle situations like this. We should probably just make imagelib handle situations like this. There are other regressions from bug 1207355 that would also probably by fixed if we fixed this.

Also, it doesn't look like Paul will get to this needinfo. For those reason's I'm gonna clear the needinfo on Paul. I think this bug should be fixed in imagelib.

	Paul Rouget [:paul]
	Comment 14 • 8 years ago

It most likely happens because of this:

http://mxr.mozilla.org/mozilla-central/source/devtools/client/responsivedesign/responsivedesign-child.js?force=1#142

> 143     // Force presContext destruction
> 144     let isSticky = docShell.contentViewer.sticky;
> 145     docShell.contentViewer.sticky = false;
> 146     docShell.contentViewer.hide();
> 147     docShell.contentViewer.show();
> 148     docShell.contentViewer.sticky = isSticky;

This is needed because we inject a special stylesheet for floating scrollbars (like on mobile), which is only taken into account if the presContext is recreated.

That was an easy workaround that I wrote years ago. Maybe there's a better way to do that now.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 15 • 8 years ago

We could still take a patch in aurora but it's too late for beta at this point.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 16 • 8 years ago

RDM will ship in 51, since this is a P3 issue, setting this to fix-optional for both 50 and 51.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 17 • 8 years ago

I was unable to repro this on DevEd51 on win10.

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 8 years ago

Confirmed via mozregression that this was fixed by bug 1273455.

	Daniel Holbert [:dholbert]
	Comment 19 • 8 years ago

Yup -- let's just call this a dupe. (Both bugs are about images blinking on relayout; both are regressions from bug 1207355; both are fixed by the same commit.)

	Daniel Holbert [:dholbert]
	Comment 20 • 8 years ago

(In reply to Ritu Kothari (:ritu) from comment #16)
> RDM will ship in 51

(To clarify for anyone who was confused by this, like I initially was -- here, Ritu was talking about a Responsive Design Mode rewrite-in-html project (which started in bug 1239437 and seems to be ongoing). As it happens, this bug isn't related to that rewrite, so its shipping schedule wouldn't impact this bug.  But happily, this is fixed anyway, so it doesn't matter. :))