1646570 - Fix GetInProcessScriptableTop usage in ImageCacheKey::GetIsolationKey

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, defect, P1)

Description

[Kris Maglione [:kmag]]

It won't do the right thing under Fission.

Comment 1

[Steven Englehardt [:englehardt]]

ni tim/dimi for prioritization.

Comment 2

[Dimi Lee [:dimi]]

By just quickly looking from the code, it seems the document calls GetInProcessScriptableTop must be a first-party document, and that means we should still be able to get the right top in Fission.
If my understanding is correct, maybe we don't really have to "fix" anything here, we can just add comments or use more appropriate API if replacing all GetInProcessXXX is the goal.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

If you call GetInProcessScriptableTop from an out-of-process subframe, it won't return nullptr, but will instead return the closest process root (e.g. if you had A1 -> B1 -> A2 -> A3 and called it on A3, it would return A2)

I don't know what the desired behaviour is here, but that doesn't seem right to me. I think some inspection of the exact semantics which you want to have in out-of-process iframes should be done here.

Comment 4

[Neha Kochar [:neha]]

Dimi, could you re-review using info from Nika in comment 3 ?

Comment 5

[Dimi Lee [:dimi]]

(In reply to Neha Kochar [:neha] from comment #4)

Dimi, could you re-review using info from Nika in comment 3 ?

Yes, I think right now, the code itself doesn't introduce any bug based on its semantics (only need to get the basedomain from the top).
But I'll work on this bug to use a more appropriate API or maybe we can just remove using it.

Comment 6

[Neha Kochar [:neha]]

(In reply to Dimi Lee [:dimi][:dlee] from comment #5)

Yes, I think right now, the code itself doesn't introduce any bug based on its semantics (only need to get the basedomain from the top).
But I'll work on this bug to use a more appropriate API or maybe we can just remove using it.

The basedomain may not be correct in out-of-process case. If we can get away with not using this API altogether, that's fine. Or if we need this info, then we need to figure out the correct way to get it and not using the InProcess... API. Please fix this for M6c.

Comment 7

[Dimi Lee [:dimi]]

(In reply to Neha Kochar [:neha] from comment #6)

The basedomain may not be correct in out-of-process case. If we can get away with not using this API altogether, that's fine. Or if we need this info, then we need to figure out the correct way to get it and not using the InProcess... API. Please fix this for M6c.

What I meant was, since we only call this API in a first-party frame and we only get the base domain from the top (will get A2 in neka's example and basedomain of A2 should be the same as the basedomain of real top A1), so it shouldn't introduce any bug.
But yes, we should still fix this with the right API, I'll work on it next week. Keep the needinfo to remind myself.

Comment 8

[Dimi Lee [:dimi]]

Attachment: Bug 1646570 - Remove using GetInProcessScriptableTop in ImageCacheKey::GetIsolationKey r=timhuang

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c1ac27f9fca
Remove using GetInProcessScriptableTop in ImageCacheKey::GetIsolationKey r=timhuang

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/1c1ac27f9fca