Closed Bug 1061257 Opened 6 years ago Closed 6 years ago

Limit LockSetForegroundWindow() to Flash


(Core :: Plug-ins, defect, P2)

Windows XP





(Reporter: gfritzsche, Assigned: aklotz)



Bug 768802 has us doing LockSetForegroundWindow() during plugin startup.
I don't remember any reports about the issue we are trying to fix with other plugins, so we shouldn't risk foreground issues and just limit this to Flash only.
Aaron, can you take this?
Flags: needinfo?(aklotz)
Priority: -- → P2
Sorry, but if you're going to limit this to Flash: Do we actually need this and the fix for bug 768802 at all?

The loosing of focus should be fixed with the release of Flash Player 15. It is fixed in beta already (see comments on the Flash Player bug [1])

Thanks for the heads-up, the bug is set to Open/ToTrack though - Jeromie, do you know if there is actually a fix for this coming up in FP15?
Flags: needinfo?(jeclark)
Our internal bugbase indicates that a fix for 3223393 will ship in the Market release.
Flags: needinfo?(jeclark)
Thanks Jeromie, does this have a timeframe?
Based on the comments I saw in bug 768802, I think that a lot more has been read into the semantics of LockSetForegroundWindow than is actually there:

* It focuses entirely on *programmatic* foreground setting; user activity always overrides restrictions on SetForegroundWindow;
* SetForegroundWindow locks have a built-in timeout;
* The changes in Z-order were triggered by uxtheme.dll in the plugin container; while presumably Flash has a dependency on uxtheme.dll causing it to be loaded, any other plugins that also have that dependency might also be affected;
* We don't have any evidence to the contrary that the foreground lock breaks anything;
* I'd be very suspicious of a plugin intentionally requesting foreground during NP_Initialize; to me it makes more sense to do something like that later in its lifecycle.

I'm inclined to resolve this bug but I'm open to discussion.
Assignee: nobody → aklotz
Flags: needinfo?(aklotz)
I've been on vacation for a couple weeks (still out, technically) and I came back to a national holiday, so I haven't had a chance to get final confirmation on an exact date yet.  Flash Player releases on a monthly cadence, so it should be very soon.

As always, we're open to engaging with Mozilla at an engineering level.  If you would like to collaborate on a better fix, I'm sure we'd be more than happy to set something up.
(In reply to Aaron Klotz [:aklotz] from comment #6)
> Based on the comments I saw in bug 768802, I think that a lot more has been
> read into the semantics of LockSetForegroundWindow than is actually there:

Ok, some of that context would have been nice on bug 768802 :)
I saw that it only blocks what Windows sees as "programmatic" foreground changes, i was thinking of intentional and desired programmatic focus changes by other applications - what e.g. about focus-changes resulting from systray context menu activity?
Couldn't we block expected behavior here? As an extreme case we can run into, think of: plugin instantiation -> plugin crash -> page triggers re-instantiation -> ...
In light of Flash 15 I've filed bug 1066182 to back out the fix from bug 768802 in its entirety.
Closed: 6 years ago
Resolution: --- → WONTFIX

Although I directly wrote to various people (including QA) on bug 768802 (because comments are not allowed) nobody cared.

In fact I'm afraid FlashPlayer 15 *does not* solve all issues. It might solve the issue with loosing focus, but under several circumstances (for which I will shortly open a new bug) focus is still lost temporarily or even restored to the wrong Firefox window!

I therefore hardly reccomend to *not* back out this patch, since it actually fixes all of the issues, instead of only concealing the most obvious ones as the Adobe hack in FlashPlayer 15 does.
Flags: needinfo?(aklotz)
OK, shame on me...

Fiorget everything I just said: I just got home from work so I could test the issue again and it turns out the update of flash failed the other day.

Apologies are in place for everyone I bugged because of this.
Flags: needinfo?(aklotz)
You need to log in before you can comment on or make changes to this bug.