Closed Bug 1234276 Opened 5 years ago Closed 4 years ago

Images on https://translate.google.com/ blink every time I enter/exit Responsive Design Mode

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect, P3)

defect

Tracking

()

RESOLVED DUPLICATE of bug 1273455
mozilla50
Tracking Status
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 + wontfix
firefox49 + wontfix
firefox50 --- fixed

People

(Reporter: arni2033, Unassigned)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

>>>   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
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?
Flags: needinfo?(seth)
Has STR: --- → yes
(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.
Reproducible on:   Win7_64, Nightly 48, 32bit, ID 20160313030418
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.)
can we fix or back out asap?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(tnikkel)
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.)
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?
Flags: needinfo?(tnikkel)
Flags: needinfo?(paul)
(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.
Flags: needinfo?(bugs)
(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
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?
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.
Flags: needinfo?(paul)
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.
We could still take a patch in aurora but it's too late for beta at this point.
Priority: -- → P3
Severity: normal → minor
RDM will ship in 51, since this is a P3 issue, setting this to fix-optional for both 50 and 51.
Flags: needinfo?(seth.bugzilla)
I was unable to repro this on DevEd51 on win10.
Confirmed via mozregression that this was fixed by bug 1273455.
Assignee: nobody → bugs
Status: NEW → RESOLVED
Closed: 4 years ago
Depends on: 1273455
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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.)
Assignee: bugs → nobody
No longer blocks: 1207355
No longer depends on: 1273455
Resolution: FIXED → DUPLICATE
Duplicate of bug: 1273455
(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. :))
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.