Closed Bug 1459335 Opened 6 years ago Closed 6 years ago

Repeated failed function hook attempts cause exhaustion of interceptor trampoline space

Categories

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

61 Branch
Unspecified
Windows
defect

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: philipp, Assigned: handyman)

References

Details

(Whiteboard: inj+)

Attachments

(1 file)

This bug was filed from the Socorro interface and is report bp-075ea3bc-7eb4-4b01-8703-679530180504. ============================================================= Top 10 frames of crashing thread: 0 xul.dll mozilla::interceptor::VMSharingPolicyUnique<mozilla::interceptor::MMPolicyInProcess, 128>::GetNextTrampoline mozglue/misc/interceptor/VMSharingPolicies.h:38 1 xul.dll mozilla::interceptor::WindowsDllDetourPatcher<mozilla::interceptor::VMSharingPolicyUnique<mozilla::interceptor::MMPolicyInProcess, 128> >::CreateTrampoline mozglue/misc/interceptor/PatcherDetour.h:402 2 xul.dll mozilla::interceptor::WindowsDllDetourPatcher<mozilla::interceptor::VMSharingPolicyUnique<mozilla::interceptor::MMPolicyInProcess, 128> >::AddHook mozglue/misc/interceptor/PatcherDetour.h:166 3 xul.dll mozilla::interceptor::WindowsDllInterceptor<mozilla::interceptor::VMSharingPolicyUnique<mozilla::interceptor::MMPolicyInProcess, 128> >::AddHook mozglue/misc/nsWindowsDllInterceptor.h:192 4 xul.dll mozilla::plugins::BasicFunctionHook<15, int >::Register dom/plugins/ipc/FunctionHook.h:168 5 xul.dll mozilla::plugins::PluginModuleChild::AllocPPluginInstanceChild dom/plugins/ipc/PluginModuleChild.cpp:1821 6 xul.dll mozilla::plugins::PPluginModuleChild::OnMessageReceived ipc/ipdl/PPluginModuleChild.cpp:622 7 xul.dll mozilla::ipc::MessageChannel::DispatchAsyncMessage ipc/glue/MessageChannel.cpp:2141 8 xul.dll mozilla::ipc::MessageChannel::DispatchMessageW ipc/glue/MessageChannel.cpp:2071 9 xul.dll mozilla::ipc::MessageChannel::RunMessage ipc/glue/MessageChannel.cpp:1917 ============================================================= crashes with this signature are newly appearing in firefox 61 on windows, around half of them in the plugin process.
Flags: needinfo?(aklotz)
The main issue that I am seeing in these dumps is that the interceptor's AddHook method is failing, but BasicFunctionHook doesn't distinguish between "we haven't hooked this yet" and "we tried to hook this but failed." Every time a new plugin instance is created, we re-attempt to set the hook. Each attempt fails, but each attempt also consumes a trampoline, until the trampoline space is eventually exhausted. While adding a more sophisticated allocator for interceptor trampolines could fix that, I think that would be overkill given the nature of the problem, and is better addressed at the BasicFunctionHook level. ni? over to David to see what he thinks.
Component: General → Plug-ins
Flags: needinfo?(aklotz) → needinfo?(davidp99)
Summary: Crash in mozilla::interceptor::VMSharingPolicyUnique<T>::GetNextTrampoline → Repeated failed function hook attempts cause exhaustion of interceptor trampoline space
I have created bug 1460002 as a meta to track similar problems in other areas of the code. Non-plugin crashes with the same signature will be fixed there.
Crash Signature: [@ mozilla::interceptor::VMSharingPolicyUnique<T>::GetNextTrampoline]
FYI, david is on pto. He'll be back next week.
Priority: -- → P3
Whiteboard: inj+
Flags: needinfo?(davidp99)
Keywords: regression
Assignee: nobody → davidp99
Priority: P3 → P1
This just does the straight-forward thing of distinguishing between having never attempted to register vs having failed to register. I had also wanted to move the initialization of the interceptions (the call to HookFunctions) from PluginModuleChild::AllocPPluginInstanceChild since, fundamentally, the mistake is that we keep trying to initialize every time we need a new PPluginInstance but the quirks system looks like it got mangled in the e10s move so we are doing some sketchy stuff about re-initialization that I _think_ is all moot anyway but, in the end, I decided it's not important enough to risk bothering. Still, this implementation is kind of a cop-out. --- Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac1abcb7feece928d541cd08a9572d8a3ed92813
Attachment #8980137 - Flags: review?(aklotz)
Attachment #8980137 - Flags: review?(aklotz) → review+
Keywords: checkin-needed
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2f9400fe51c4 Distinguish between unregistered and failed DLL function interceptions. r=aklotz
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: