1446499 - Interceptor in FunctionHook::HookProtectedMode should be persistent

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[(No longer employed by Mozilla) Aaron Klotz]

The interceptor in FunctionHook::HookProtectedMode is allocated on the stack. When destroyed, that interceptor will fix up its trampolines such that the hooks are effectively inert.

That interceptor needs to live beyond the lifetime of HookProtectedMode's scope.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

David, could you take a look at this please?

Comment 2

[Jim Mathies [:jimm]]

Hey Aaron, what's the priority on this?

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

This does break pref-based disabling of Protected Mode in 32-bit flash.

I don't know what the current state of Flash Sandboxing is on 32-bit Windows, but if we intend to override their sandbox by default, this should be high priority. Otherwise it's not too urgent.

Comment 4

[David Parks [:handyman]]

I won't have time this week but should be able to take this later.

Comment 5

[David Parks [:handyman]]

Attachment: Use static nsWindowsDllInterceptor in plugin's HookProtectedMode

There was already an nsWindowsDllInterceptor intended for this purpose -- this was essentially a cut-and-paste fail.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6147dace3d0e
Use static nsWindowsDllInterceptor in plugin's HookProtectedMode r=aklotz

Comment 7

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/6147dace3d0e

Comment 8

[David Parks [:handyman]]

Comment on attachment 8965044 [details] [diff] [review]
Use static nsWindowsDllInterceptor in plugin's HookProtectedMode

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1382251 refactored the nsWindowsDllInterceptor usage and introduced this regression

[User impact if declined]:
The Adobe Protected Mode sandbox (the 32 bit sandbox for Flash) cannot be disabled in about:addons.

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
This small change restores long-standing behavior to the way it existed before the refactor.

[String changes made/needed]:
None

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 8965044 [details] [diff] [review]
Use static nsWindowsDllInterceptor in plugin's HookProtectedMode

flash sandboxing fix, approved for 60.0b12

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d464bc1927f0

Comment 11

[Andrei Vaida [:avaida]]

Setting qe-verify- based on David's assessment on manual testing needs (Comment 8).