1433883 - Remove the weak pres context pointer from ContentEventHandler.

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

Holding a weak pres context pointer across stuff that does flushes is dangerous.

Hopefully, we just poke at it when we find a frame (thus a pres context should be around, and it rather be the one that we started poking at).

I think it'd be better to just not keep the member around, since frames can reach the pres context easily.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1433883: Remove weak pres context pointer from ContentEventHandler.

Holding a weak pres context pointer across stuff that does flushes is dangerous.

Hopefully, we just poke at it when we find a frame (thus a pres context should
be around, and it rather be the one that we started poking at).

I think it'd be better to just not keep the member around, since frames can
reach the pres context easily.

Review commit: https://reviewboard.mozilla.org/r/216194/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216194/

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8946220 [details]
Bug 1433883: Remove weak pres context pointer from ContentEventHandler.

https://reviewboard.mozilla.org/r/216194/#review221994

::: dom/events/ContentEventHandler.cpp:519
(Diff revision 1)
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  nsPresContext* presContext = frame->PresContext();
> +
>    // account for any additional frames
> -  while ((frame = frame->GetNextContinuation()) != nullptr) {
> +  while ((frame = frame->GetNextContinuation())) {

nit: remove unnecessary ().

::: dom/events/ContentEventHandler.cpp:2181
(Diff revision 1)
>        rect = LayoutDeviceIntRect::FromUnknownRect(
> -               charRect.ToOutsidePixels(mPresContext->AppUnitsPerDevPixel()));
> +               charRect.ToOutsidePixels(baseFrame->PresContext()->AppUnitsPerDevPixel()));

nit: over 80 characters. please fix it before landing.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8946220 [details]
Bug 1433883: Remove weak pres context pointer from ContentEventHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216194/diff/1-2/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8946220 [details]
Bug 1433883: Remove weak pres context pointer from ContentEventHandler.

https://reviewboard.mozilla.org/r/216194/#review221994

> nit: remove unnecessary ().

I cannot remove this one because clang warns with -Werror it asks you to put the double parens to disambiguate from `frame == frame->GetNextContinuation()`.

> nit: over 80 characters. please fix it before landing.

Done, thanks a lot for the review! :)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0255a42c0eb9
Remove weak pres context pointer from ContentEventHandler. r=masayuki

Comment 6

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/0255a42c0eb9