1460003 - nsWindow::UpdateGetWindowInfoCaptionStatus: AddHook on GetWindowInfo should only be invoked once

Status: RESOLVED FIXED

(Core :: Widget: Win32, enhancement, P1)

Description

[(No longer employed by Mozilla) Aaron Klotz]

Otherwise we repeatedly try the hook over and over.

Comment 1

[Jim Mathies [:jimm]]

https://searchfox.org/mozilla-central/rev/f30847c12e5fb13791401ed4330ced3e1b9c8d81/widget/windows/nsWindow.cpp#2494

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch

Comment 3

[Jim Mathies [:jimm]]

Since mHookedGetWindowInfo is a member of nsWindow the code will retry AddHook for each window object. Not sure if that's desired behavior.. do we anticipate that one AddHook failure implies all other attempts will also fail?

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Jim Mathies [:jimm] from comment #3)
> Since mHookedGetWindowInfo is a member of nsWindow the code will retry
> AddHook for each window object. Not sure if that's desired behavior.. do we
> anticipate that one AddHook failure implies all other attempts will also
> fail?

Typically, yes. If AddHook fails, it is usually because some instructions in the binary are encountered that the interceptor does not understand. That situation is unlikely to improve with subsequent invocations.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

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

LGTM

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

But yeah, I guess it makes less sense for mHookedGetWindowInfo to be per-nsWindow. There really should only ever be one.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

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

Review of attachment 8976211 [details] [diff] [review]:
-----------------------------------------------------------------

Changing my review here to reflect my earlier comments.

::: widget/windows/nsWindow.h
@@ +679,5 @@
>    // Pointer events processing and management
>    WinPointerEvents mPointerEvents;
> +
> +  // AddHook success checks
> +  mozilla::Maybe<bool> mHookedGetWindowInfo;

Yeah I think this should be static (or equivalent)

Comment 8

[Jim Mathies [:jimm]]

Attachment: patch v.2

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 8976233 [details] [diff] [review]
patch v.2

updated

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8976233 [details] [diff] [review]
patch v.2

Review of attachment 8976233 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +351,5 @@
>  // General purpose user32.dll hook object
>  static WindowsDllInterceptor sUser32Intercept;
>  
> +// AddHook success checks
> +mozilla::Maybe<bool> sHookedGetWindowInfo;

Missing static

Comment 11

[Jim Mathies [:jimm]]

Attachment: patch v.3

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d8c17bc3145
Only call nsWindow's AddHook for GetWindowInfo once. r=aklotz

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8d8c17bc3145