1129267 - Google finance charts are no longer HiDPI

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benoit Girard (:BenWa)]

Link: https://www.google.com/finance?q=NASDAQ%3AINTC

I wonder if this broke with the recent plugin changes.

Comment 1

[Stephen A Pohl [:spohl]]

This sounds like side-effects of bug 1118615. Reading through the comments there, it seems like we have a choice between hanging Facebook apps and low-DPI Google Finance...

Comment 2

[Benjamin Smedberg]

smichaud? I thought bug 1118615 was side-effect-free. I suspect that if it's causing any regressions, we should back bug 1118615 out, since the peopleroulette hang is actually a bug in the SWF.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

> I thought bug 1118615 was side-effect-free.

So did I.  Apparently not.  (Though I'd appreciate it if others can double-check to make sure.)

If, after double-checking, this turns out to be true, then yes, I suppose we need to back out the patch for bug 1118615.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Yes, the patch for bug 1118615 did cause this bug.  I confirmed this by backing the patch out locally.

And yes, backing out the patch does bring back the hangs triggered by trying to use the peopleroulette site on machines that have Retina displays.

We don't know for sure that the peopleroulette hangs are caused by a bug in one of their SWF files, but that's where the evidence currently points.  We also have no good way to track the hangs by signature, so we don't know if they also happen on other sites.

On current evidence, we should back out the patch for bug 1118615.  But before we do it, I'd like (if possible) to set up some way of tracking the increase in Flash hangs/crashes that will happen as a result.

Kairo, do you have any suggestions or advice on this?  Can you think of anyone else to ask?  I'm going to try to do the backouts simultaneously on the 38, 37 and 36 branches, so we'll have a single starting point to measure from.

Comment 5

[Benoit Girard (:BenWa)]

Are you concerned about a specific SWF file? We could only trigger the quirk if we hit the stream URL of the SWF (or however we load the SWF). It's a practical solution but it's a terrible precedent to work around buggy SWF by shipping quirks. But practical solution > technical purity.

We could reach out to the developers as well.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

I rather like your suggestion, Benoit :-)

Yes, it's ugly.  But if the problem really is a single bad SWF file (or a single bad site), it would get us out of a very uncomfortable situation.

I'd still want to try to measure any increase in Flash hangs or crashes, in case the problem really isn't limited to that single site.

It might be easier to simply hide HiDPI support for all Flash objects that have "peopleroulette" (or something like that) in them.

> We could reach out to the developers as well.

The owner of the site is some company in Panama -- even though it's an English language site.  This might be more trouble than it's worth.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

>> We could reach out to the developers as well.
>
> The owner of the site is some company in Panama -- even though it's
> an English language site.  This might be more trouble than it's
> worth.

There's also the more valid problem that the bug is apparently in
something we know almost nothing about -- a Flash ActionScript.  Adobe
would be better equipped to contact the developers, but it's not their
policy to do that.  See bug 1118615 comment #56.

Comment 8

[Robert Kaiser]

(In reply to Steven Michaud from comment #4)
> Kairo, do you have any suggestions or advice on this?  Can you think of
> anyone else to ask?  I'm going to try to do the backouts simultaneously on
> the 38, 37 and 36 branches, so we'll have a single starting point to measure
> from.

I don't really have suggrestions, but backing out simultaneously sounds like the wrong thing to do to me. It does not help measuring anything but gives beta an infusion of instability without prior testing, doesn't it?

Comment 9

[Steven Michaud [:smichaud] (Retired)]

The peopleroulette site is unlikely to have many users on *any* of our non-release distros, including beta.  But the beta channel has the greatest likelihood of use.  And backing out something on beta is much less likely to introduce unknown factors than landing something new.

And we need to land something on beta soon to avoid this bug getting into the FF 36 release.

And in any case, I'm now in favor of simply backing out the patch for bug 1118615.  I just checked the number of crashes on OS X whose url includes "peopleroulette" over the last week -- including (of course) the release branches, which don't have the patch for bug 1118615.  I only found two.  So if bug 1118615 is limited to the peopleroulette site (as currently seems likely), we shouldn't see any significant increase in that bug's crashes when we back out its patch.

Furthermore I can no longer reproduce bug 1118615's actual hangs and crashes with the latest version of the Flash plugin, testing on FF 36b1 and FF 35.  The peopleroulette chat app still stops loading (in HiDPI mode) at the same point (with a completed progress bar displayed), but this no longer results in us killing the Flash plugin.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

The current version of the Flash plugin is 16.0.0.305.  But I also don't see these hangs/crashes (in FF 35 or 36b1) with 16.0.0.296.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Attachment: Backout patch for bug 1118615 on trunk

I've started a set of tryserver builds, here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4841a6997cb

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8561477 [details] [diff] [review]
Backout patch for bug 1118615 on trunk

Landed on fx-team:
https://hg.mozilla.org/integration/fx-team/rev/ec092b22f40d

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Attachment: Backout patch for bug 1118615 on aurora and beta

Carry forward Markus' r+.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8561504 [details] [diff] [review]
Backout patch for bug 1118615 on aurora and beta

Approval Request Comment
[Feature/regressing bug #]: bug 1118615
[User impact if declined]: Pretty bad UI bug
[Describe test coverage new/current, TreeHerder]: manual testing
[Risks and why]: Low.  Restores pre-1118615 behavior, and bug 1118615 is now mitigated by changes to most recent versions of Flash plugin.
[String/UUID change made/needed]: None

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8561504 [details] [diff] [review]
Backout patch for bug 1118615 on aurora and beta

Landed on aurora and beta:

https://hg.mozilla.org/releases/mozilla-aurora/rev/a7fca11f7b4c
https://hg.mozilla.org/releases/mozilla-beta/rev/1673712f9408

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec092b22f40d