1709460 - APZCCallbackHelper.cpp has a call to nsPresContext::GetParentPresContext() (in an assertion) which needs to be adjusted for a fission world

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

APZCCallbackHelper.cpp has this snippet:

void APZCCallbackHelper::InitializeRootDisplayport(PresShell* aPresShell) {
[...]
    // This code is only correct for root content or toplevel documents.
    MOZ_ASSERT(!pc || pc->IsRootContentDocumentCrossProcess() ||
               !pc->GetParentPresContext());

If we're in a cross-origin iframe, then the call to pc->GetParentPresContext() will trivially fail (due to not being able to traverse a process boundary). This effectively nerfs the assertion and makes it trivially pass without checking anything useful.

The assertion is, in fact, meant to verify that we're not in an iframe, but in fact it'll trivially pass if we're in a cross-origin iframe (in a session with fission enabled).

We need to adjust the assertion to account for this.

Comment 1

[Daniel Holbert [:dholbert]]

This bug probably doesn't need to block any particular fission milestone, since it doesn't impact user-perceptible behavior. It's just about a MOZ_ASSERT debug-only check that's inadvertently becoming less strict with fission enabled, basically.

Comment 2

[Timothy Nikkel (:tnikkel)]

Yes, but we also have to audit that we don't call this function in a way that would cause a new, modified assertion to fire.

Comment 3

[Botond Ballo [:botond]]

This is similar to bug 1698693 (an audit of code to make sure it works fine with Fission), so tracking it with the same priority (M7a).

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1709460 - Add a flag representing whether mEffectsInfo has been set or not. r?tnikkel

We are going to use the mEffectsInfo's mVisibleRect as a displayport base for
OOP iframe's root scroller and we don't want the displayport base if it hasn't
been arrived from the parent content, to do that, we need to tell the difference
whether the iframe was painted as empty or hasn't been painted.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1709460 - Try to initialize a displayport base for OOP iframe's root scroller with its visible rect. r?tnikkel

In the same manner what we initialize a diplayport base for top level contents.

Depends on D116575

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1709460 - Use OOP iframe's visible rect as a displayport base. r?tnikkel

Depends on D116576

Comment 7

[Hiroyuki Ikezoe (:hiro)]

As discussed with Timothy on Matrix, I dropped the last piece of changes deferred it to bug 1716436.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20c1ef91fc10
Add a flag representing whether mEffectsInfo has been set or not. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/07272d29f97f
Try to initialize a displayport base for OOP iframe's root scroller with its visible rect. r=tnikkel

Comment 9

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/20c1ef91fc10
https://hg.mozilla.org/mozilla-central/rev/07272d29f97f

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9224655 [details]
Bug 1709460 - Try to initialize a displayport base for OOP iframe's root scroller with its visible rect. r?tnikkel

Beta/Release Uplift Approval Request

• User impact if declined: Basically nothing, this change will change checkerboard severity telemetry data, with this change we will not report incorrect severity for OOP iframes in Fission.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change does just set diplayport data property when we initialize OOP iframes, it just makes things correct such as incorrect severity telemetry data, should not have impacts either on user interactions on rendering results. It may reduce the cases that users will see checkerboarding during scrolling inside OOP iframes but it's a good thing.
• String changes made/needed:

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 9224655 [details]
Bug 1709460 - Try to initialize a displayport base for OOP iframe's root scroller with its visible rect. r?tnikkel

given the limited impact I think I prefer to let this ride the trains to 91

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Oops, I forgot mentioning this will fix one of the reasons that our checkerboard severity data on fission beta experiment looks odd (bug 1713360). Neha agreed on landing this change on beta.

Though I'd like to the approval request again, but it seems that one it has been rejected we can no longer re-request. I'd do NI instead.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Timothy suggested diagnosing nightly results with/without fission instead of trying to uplift the change to beta. Yeah, I agree with that.

So, here are results, with Fission and without Fission;

Checkerboard severity with Fission on Nightly

Checkerboard severity without Fission on Nightly

As you can see the severity has dropped down with Fission since June 15, whereas it looks same as before without Fission.

I am going to attach two images for the charts because the queries above might show us different results in future.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: A chart for `with fission`

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: A chart for `without fission`

Comment 16

[Kris Maglione [:kmag]]

I'll second that. This isn't risky for users, and doesn't affect non-Fission data at all. We need it in order to be able to get any sort of remotely useful data from Fission users.

Comment 17

[Botond Ballo [:botond]]

(In reply to Kris Maglione [:kmag] from comment #16)

I'll second that. This isn't risky for users, and doesn't affect non-Fission data at all. We need it in order to be able to get any sort of remotely useful data from Fission users.

Are you seconding comment 12 ("let's uplift to beta") or comment 13 ("let's not uplift to beta")?