1570741 - AntiTrackingCommon::IsFirstPartyStorageAccessGrantedFor returns false if nsGlobalWindowOuter::GetInProcessTop() returns null during window.open

Status: RESOLVED INVALID

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

Description

[Andrew Sutherland [:asuth] (he/him)]

Attachment: crash stack from the log

:nika is encountering a reproducible failure of https://searchfox.org/mozilla-central/source/dom/html/test/test_window_open_close.html in https://treeherder.mozilla.org/#/jobs?repo=try&revision=9221d7cc52ba735aaae0eaa6297d178c34f6c181 with example failure log https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=259385046&repo=try&lineNumber=24900 with the backtrace pasted into the attachment.

The basic situation is:

• SessionStorage is asserting at https://searchfox.org/mozilla-central/rev/b38e3beb658b80e1ed03e0fdf64d225bd4a40327/dom/storage/Storage.cpp#45 because it expects the guard at https://searchfox.org/mozilla-central/rev/b38e3beb658b80e1ed03e0fdf64d225bd4a40327/dom/base/nsGlobalWindowInner.cpp#4447 to have prevented its creation.
  • This is apparently not a safe assumption because, as is happening in this crash, it's also the case that window.open can invoke GetStorage() at https://searchfox.org/mozilla-central/rev/b38e3beb658b80e1ed03e0fdf64d225bd4a40327/toolkit/components/windowwatcher/nsWindowWatcher.cpp#1176-1178 when doing storage forking, bypassing nsGlobalWindowInner::GetSessionStorage at https://searchfox.org/mozilla-central/rev/b38e3beb658b80e1ed03e0fdf64d225bd4a40327/dom/base/nsGlobalWindowInner.cpp#4371
• But things are perhaps more complicated because it appears the storage check is inconsistent per the warning WARNING: '!topWindow', file /builds/worker/workspace/build/src/toolkit/components/antitracking/AntiTrackingCommon.cpp, line 1169 at https://searchfox.org/mozilla-central/rev/b38e3beb658b80e1ed03e0fdf64d225bd4a40327/toolkit/components/antitracking/AntiTrackingCommon.cpp#1169 because the call 2 lines above to get the top window is failing.
  • It's not immediately clear why this is happening, and unfortunately the builds this are happening on are quantum render builds, which I/we guess is owing to pernosco only attempting to reproduce in certain configurations.

As somewhat of an aside, :nika notes that it's more appropriate to get the scriptable top via https://searchfox.org/mozilla-central/rev/b38e3beb658b80e1ed03e0fdf64d225bd4a40327/dom/base/nsGlobalWindowOuter.cpp#3104 rather than the top because scriptable top stops at mozbrowser boundaries, and this seems like the right semantic thing, even if mozbrowser is going away (gone?).

:baku, do you have thoughts on this problem? One thing I'm wondering is whether antitracking should be upgrading the warn_if failure to an assertion to make it more obvious if callers are triggering logic at times when fundamental invariants don't hold.

There's probably also a spin-off for additional logic guards in the window opening logic and perhaps in the logic that pushes data into sessionstorage from sessionstore at https://searchfox.org/mozilla-central/rev/b38e3beb658b80e1ed03e0fdf64d225bd4a40327/toolkit/components/sessionstore/SessionStoreUtils.cpp#993-994, although that might currently be working out okay if we don't try and create the sessionstorage if we don't have any persisted data. (Although it could cause problems for restored tabs if the permissions or tracking databases have changed since they were persisted?)

Comment 1

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

I have made changes (quite a few of them) in that patch stack, so I'm likely responsible for whatever change caused the assumptions to fall apart. I originally reached out to :asuth because I couldn't figure out how the assertions were correct in the first place. I'm guessing the failures are related to the changes in bug 1523638.

As somewhat of an aside, :nika notes that it's more appropriate to get the scriptable top via https://searchfox.org/mozilla-central/rev/b38e3beb658b80e1ed03e0fdf64d225bd4a40327/dom/base/nsGlobalWindowOuter.cpp#3104 rather than the top because scriptable top stops at mozbrowser boundaries, and this seems like the right semantic thing, even if mozbrowser is going away (gone?).

I'd much prefer it if you actually just switch to using BrowsingContext. Fiddling around with the specifics of the old-style methods isn't super important at this point :-P.

Comment 2

[(no longer active)]

(In reply to :Nika Layzell (Away until Aug. 26) from comment #1)

I have made changes (quite a few of them) in that patch stack, so I'm likely responsible for whatever change caused the assumptions to fall apart. I originally reached out to :asuth because I couldn't figure out how the assertions were correct in the first place. I'm guessing the failures are related to the changes in bug 1523638.

As somewhat of an aside, :nika notes that it's more appropriate to get the scriptable top via https://searchfox.org/mozilla-central/rev/b38e3beb658b80e1ed03e0fdf64d225bd4a40327/dom/base/nsGlobalWindowOuter.cpp#3104 rather than the top because scriptable top stops at mozbrowser boundaries, and this seems like the right semantic thing, even if mozbrowser is going away (gone?).

I'd much prefer it if you actually just switch to using BrowsingContext. Fiddling around with the specifics of the old-style methods isn't super important at this point :-P.

I've filed bug 1575609 to do this part. I'll let Andrea answer the rest of the questions here.

Comment 3

[Cosmin Sabou [:CosminS]]

Hey, any updates here? Bug 1533759 has 147 failures in the last 7 days and 402 in the last 30 days, it's one of the most frequent bugs being also in the disable recommended list: https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?startday=2019-08-22&endday=2019-08-29&tree=trunk&bug=1533759

https://bugzilla.mozilla.org/show_bug.cgi?id=1533759#c42
Since Andrea is not responding to the ni 28 days now, could someone else take over this bug? Nika, Ehsan? Thank you.

Comment 4

[(no longer active)]

Perhaps Andrew would be able to help, I really don't know much about comment 0 besides what I already tried to help with here.

Comment 5

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

I can't contribute much here, leaving ni? for asuth.

Comment 6

[Andrea Marchesini [:baku]]

The code has changed a lot recently. Tim, do you think this bug is still valid?

Comment 7

[Tim Huang[:timhuang]]

No, I don't tink this bug is still valid. Right now, our storage access check for windows doesn't use the inProcessTop anymore.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Marking bug invalid based on comment 7.