Closed Bug 1699031 Opened 3 years ago Closed 3 years ago

s/GetCrossDocParentFrame/GetCrossDocParentFrameInProcess/ in ImageLoader.cpp

Categories

(Core :: Layout: Images, Video, and HTML Frames, task)

task

Tracking

()

RESOLVED FIXED
88 Branch
Fission Milestone M7a
Tracking Status
firefox88 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files)

Per bug 1698680, we're adding a new wrapper-API called GetCrossDocParentFrameInProcess(), and we're migrating existing GetCrossDocParentFrame() calls to use the new function, after checking that they're OK with the fact that it doesn't cross processes for cross-origin content.

ImageLoader.cpp has one such call, which I'll migrate in this bug.

So, the code I'm looking at here is https://searchfox.org/mozilla-central/rev/2cc3b39bc9bb024d35e09e9c8acecf0e2dfb4e13/layout/style/ImageLoader.cpp#571 which is about invalidating ancestors' rendering observers (like other elements using -moz-element()), after an image is invalidated.

Here's a testcase to see whether -moz-element actually works at all for a cross-origin iframe (no images involved at all).

To my surprise, this testcase does work in regular Firefox Nightly right now (with fission disabled), but it does not work if I enable fission. I think that's kinda expected, as a security feature? It does represent a behavior change, but it may be one that we're intentionally making. tnikkel, do you know if this behavior change is already known / well-understood?

Flags: needinfo?(tnikkel)

(In reply to Daniel Holbert [:dholbert] from comment #1)

To my surprise, this testcase does work in regular Firefox Nightly right now (with fission disabled),

The frame tree (by this I mean the extended cross document free tree) and hence rendering don't really know about origin (they could check, but they don't need to for anything that I know of) so if things share a frame tree moz-element can draw it.

but it does not work if I enable fission. I think that's kinda expected, as a security feature? It does represent a behavior change, but it may be one that we're intentionally making. tnikkel, do you know if this behavior change is already known / well-understood?

I'm not sure, it could be. Someone may well have thought of this issue before, but I can't find anything in bugzilla. This (the current situation) is what would happen if we just never expended any energy on this case. But that could also be what we want to do even if there was energy to expend on this for security reasons.

Flags: needinfo?(tnikkel)

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

The frame tree (by this I mean the extended cross document free tree) and hence rendering don't really know about origin (they could check, but they don't need to for anything that I know of) so if things share a frame tree moz-element can draw it.

There is tainted canvas's when we draw crossorigin images to them, but afaik we don't have anything similar for mozelement.

This patch doesn't change behavior; it's just switching us between two
functions that do the same thing. (One is literally a trivial wrapper for the
other.)

We're using the new "InProcess" version of this API as a way of annotating
callsites that have been vetted as behaving properly in out-of-process iframes.

The code here, InvalidateImages, is walking up an invalidated image's
ancestor-chain, to invalidate any rendering observers of those ancestors. It's
OK that this walk will stop at the process boundary, because we don't support
rendering observers for cross-origin out-of-process content anyway.

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d956e3a61dfe
Switch to use wrapper-API GetCrossDocParentFrameInProcess() in ImageLoader.cpp. r=tnikkel
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch

Setting Fission Milestone to M7a (the current Beta milestone) because this bug is blocking meta bug 1599913 which is a blocker for Fission M7a.

Fission Milestone: --- → M7a
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: