1733509 - PositionedEventTargeting.cpp calls GetResolution

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Timothy Nikkel (:tnikkel)]

https://searchfox.org/mozilla-central/rev/7539ad54ddc720a0553efd07ca681b9a409f9887/layout/base/PositionedEventTargeting.cpp#300

This code might need changes when fission comes to android.

Comment 1

[Chris Peterson [:cpeterson]]

(In reply to Timothy Nikkel (:tnikkel) from comment #0)

This code might need changes when fission comes to android.

In that case, I will tag this bug to be tracked for Android Fission.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

It turned out that the pres shell in AppUnitsFromMM in question is NOT the top level one, it's for the document where the event happens regardless of whether it's in OOP or not. So for example, even without Fission if I clicked inside an iframe element, the GetResolution always returns 1.0. So this bug is definitely valid, but it's not for Fission specific, it's been there since the beginning of our event retarget system. And as far as I can tell no one had reported this issue until Timothy found the code flaw.

With the reason I'd say this bug doesn't need to block Android Fission. Chris, what do you think?

Comment 3

[Timothy Nikkel (:tnikkel)]

This code is only executed on Android, where we haven't enabled fission, so we wouldn't expect anyone to find it except my reading the code.

Comment 4

[Chris Peterson [:cpeterson]]

(In reply to Timothy Nikkel (:tnikkel) from comment #3)

This code is only executed on Android, where we haven't enabled fission, so we wouldn't expect anyone to find it except my reading the code.

So it sounds like this bug is still valid, but will only affect Android Fission. If that's correct, I'll still track this bug for Android Fission.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

This bug has already been there without enabling Fission on Android, that's what I meant.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)

This bug has already been there without enabling Fission on Android, that's what I meant.

To be clear, Android users should be able to find this bug without enabling Fission by clicking somewhere inside an iframe on a zoomed in/out content. With the content, event-retargetting doesn't work as what the code is supposed to. I presume people isn't so severe where event re-targetting happened or not.

Comment 7

[Chris Peterson [:cpeterson]]

Thanks for the clarification. In that case, I won't track this bug as a blocker for Android Fission.

Comment 8

[Timothy Nikkel (:tnikkel)]

My apologies Hiro, I didn't read comment 2 closely enough.

Comment 9

[Alex Jakobi [:ajakobi]]

Attachment: Bug 1733509 - Use TryInferEnclosingResolution for event retargeting. r=botond

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6affdb4f3016
Use TryInferEnclosingResolution for event retargeting. r=botond

Comment 11

[Sandor Molnar[:smolnar]]

Backed out for causing gv-junit crashes @ TryInferEnclosingResolution

Backout link: https://hg.mozilla.org/integration/autoland/rev/5a31505f08c64efa6eb7a8ed02d5e9c0db448e37

Push with failures

Failure log -> PROCESS-CRASH | MOZ_ASSERT(!aShell->GetPresContext()->GetParentPresContext()) (TryInferEnclosingResolution can only be called for a root pres shell within a process)

Comment 12

[Botond Ballo [:botond]]

Failure log -> PROCESS-CRASH | MOZ_ASSERT(!aShell->GetPresContext()->GetParentPresContext()) (TryInferEnclosingResolution can only be called for a root pres shell within a process)

I guess the in-process root content document (what we are passing to TryInferEnclosingResolution) is not necessarily always the root of the entire document hierarchy in the process (what TryInferEnclosingResolution is asserting), i.e. sometimes there is a chrome document enclosing the root content document.

(It's interesting to note the failure is in ExtensionActionTest#testOpenPopup, suggesting that the scenario where this doesn't hold is related to popups.)

Suggested fix: make an additional call to GetRootPresContext() before calling TryInferEnclosingResolution().

(Note: we don't want to replace the call to GetInProcessRootContentDocumentPresContext() with GetRootPresContext(), because for the resolution we do want the root content document.)

Comment 13

[Hiroyuki Ikezoe (:hiro)]

I think TryInferEnclosingResolution was originally supposed to be called in a content process.

And it looks like the JUnit test runs with extensions.webextensions.remote=false (which is not our default value), thus the popup document gets loaded in the parent process as a content document.

If we allow such situations, I think we can just drop the assertion entirely from the function, this IsTopLevel() check handles the situations properly.

Comment 14

[Alex Jakobi [:ajakobi]]

Thanks, Hiro. Removing the assertion of course seems to be the easiest fix. Botond, what do you think?

Comment 15

[Botond Ballo [:botond]]

(In reply to Alex Jakobi [:ajakobi] from comment #14)

Removing the assertion of course seems to be the easiest fix. Botond, what do you think?

I think the assertion is helpful to catch the scenario where the caller passes in a subdocument in the content process of an OOP iframe. In that scenario, the OOP iframe may in fact be subject to an enclosing resolution, but the function would just silently return 1.0f instead.

As an alternative suggestion, perhaps we can add an early-return if (!XRE_IsContentProcess()) to the function (before the assertion)?

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5fd9adde57f1
Use TryInferEnclosingResolution for event retargeting. r=botond

Comment 17

[Aron Cseh]

https://hg.mozilla.org/mozilla-central/rev/5fd9adde57f1