825620 - Regression: Flash is triggering the discrete GPU

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Girard (:BenWa)]

Flash used to correctly work on the integrated GPU but this regressed.

STR:
1) Install gfxCardStatus
2) Close any app listed as using the discrete GPU
3) Open FF and navigate to youtube.

Expected: Youtube shouldn't trigger a change to the discrete GPU ('d' icon in gfxCardStatus)

Perhaps we regressed this with our recent changes to the plugin code. It's been at least a few weeks since we regressed this.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch

What happens is AddQuirks would be called before we initialized the quirks. This patch moves the quirk with the other OSX quirks.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 697029 [details] [diff] [review]
patch

Good catch!

This looks fine to me, but I do have one minor quibble:

My original patch worked around a bug in the Flash plugin, and checked for all plugin "descriptions" starting with "Shockwave Flash".  But your patch checks for the "application/x-shockwave-flash" MIME type -- which means that my workaround would be applied to any plugin that handled that MIME type.

My patch for bug 804606 is harmless (for any plugin, implementing any MIME type).  And as far as I know there aren't any other plugins available for OS X that handle that MIME type.  But I'm puzzled that you chose to check the MIME type and not the description.

Comment 3

[Benoit Girard (:BenWa)]

My gut feeling is there's no real restriction on what plugins can/should use for their description. An unrelated plugin could have a description such as 'This plug-in provides video playback like Shockwave Flash' or whatnot which could include the substring if it refers to flash, while the mime type is a better indicator.

Comment 4

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da905fbd3113

Comment 5

[Ed Morley [:emorley]]

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

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Turns out I made a mistake in my patch for bug 804606 that this patch (inadvertently) fixes.  This line can cause nsDependentCString() to be called on a NULL value:

http://hg.mozilla.org/releases/mozilla-release/rev/3fbda0a1a6e9?revcount=120#l2.26

See bug 828954 comment #14.

So this patch will need to get onto the beta branch pretty soon.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 697029 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 804606
User impact if declined: Crash/hang for all users of Unity plugin (bug 828954)
Testing completed (on m-c, etc.): On m-c for a week (and aurora for a few days) with no problems
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

I assume this is alright with you Benoit.  Please comment if you disagree.

Comment 8

[Benoit Girard (:BenWa)]

No objections, please go ahead.

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 697029 [details] [diff] [review]
patch

Approving for Beta 19, in part as preparation for possible uplift to an 18.0.1.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/808cf2db3fff

Comment 11

[bhavana bajaj [:bajaj]]

Steven, we are considering to fix Bug 828954 if we do a 18.0.1,  can we please get a patch ready for release & nominate it for approval-mozilla-release in preparation ? Thanks !

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Bug 828954 is already fixed by this bug's patch.

Bug 829284 is another matter, though.  I'm working on it, but haven't yet come up with a solution.  It's hard to predict how long I'll require.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 697029 [details] [diff] [review]
patch

Oops, I misunderstood comment #11.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 804606
User impact if declined: Crash/hang for all users of Unity plugin (bug 828954)
Testing completed (on m-c, etc.): On m-c for a week (and aurora for a few days) with no problems
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 14

[bhavana bajaj [:bajaj]]

Comment on attachment 697029 [details] [diff] [review]
patch

Please go ahead and land this on mozilla-release to help fix Bug 828954 which is needed for FF18.0.1

Comment 15

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-release/rev/0c3fcdd17352

Comment 16

[Bogdan Maris, Desktop Test Engineering]

@Benoit Girard or someone with dual GPU on mac. Can you verify this bug on 19 beta 3?

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/19.0b3-candidates/build1/mac/en-US/

I don`t have the hardware necessary to verify it myself

Comment 17

[Benoit Girard (:BenWa)]

(In reply to Bogdan Maris [QA] from comment #16)
> @Benoit Girard or someone with dual GPU on mac. Can you verify this bug on
> 19 beta 3?
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/19.0b3-candidates/
> build1/mac/en-US/
> 
> I don`t have the hardware necessary to verify it myself

verified

Comment 18

[Bogdan Maris, Desktop Test Engineering]

Can you also please verify the issue on Firefox 20 beta 1?
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/20.0b1-candidates/build2/mac/en-US/

Comment 19

[Mihai Morar, (:MihaiMorar)]

(In reply to Bogdan Maris [QA] from comment #18)
> Can you also please verify the issue on Firefox 20 beta 1?

I confirm the fix is verified on Mac OS 10.7 (FF 20.b5):

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:20.0) Gecko/20100101 Firefox/20.0(20130313170052)

Comment 20

[Mihai Morar, (:MihaiMorar)]

Benoit can you please confirm this is fix too?

Comment 21

[Bogdan Maris, Desktop Test Engineering]

Can someone with dual GPU please verify this using Firefox 21 beta 3 as well?

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/21.0b3-candidates/build1/mac/en-US/

Comment 22

[Benoit Girard (:BenWa)]

confirmed with that link.