Perma toolkit/components/antitracking/test/browser/browser_partitionedLocalStorage_events.js | No events - 1 == 0 when Gecko 68 merges to beta on 2019-07-01

RESOLVED FIXED in Firefox 69

Status

()

defect
RESOLVED FIXED
3 months ago
27 minutes ago

People

(Reporter: aryx, Assigned: Ehsan)

Tracking

(Regression, Regressed 1 bug, {regression})

unspecified
mozilla69
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox67 unaffected, firefox67.0.1 unaffected, firefox68 unaffected, firefox69+ fixed)

Details

Attachments

(1 attachment)

This has been noticed first in a central-as-beta simulation on May 31st: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception&revision=073df4aa87c5ae9d2cf2432e0435e772e7baf7ff&searchStr=mochitest&selectedJob=249377846
It got tracked together with a localStorage issue in bug 1556093 and got split off into this one.

It only affects late beta simulations from a first investigation.

It's also part of today's simulation: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=c2d8fe0f14155a85e80e13e21bee8867356ca027&selectedJob=249946254

The failures have slightly changed since it appeared first, now it's https://treeherder.mozilla.org/logviewer.html#?job_id=249946254&repo=try

14:59:57 INFO - Let's see if non-tracker page has received events
14:59:57 INFO - Getting the value...
14:59:57 INFO - TEST-PASS | toolkit/components/antitracking/test/browser/browser_partitionedLocalStorage_events.js | The value is correctly set by the tracker - true == true -
14:59:57 INFO - Getting the events...
14:59:57 INFO - Buffered messages finished
14:59:57 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/antitracking/test/browser/browser_partitionedLocalStorage_events.js | No events - 1 == 0 -

Flags: needinfo?(ehsan)

Oh, thanks a lot! I actually never realized this is a perma failure, I thought this is an intermittent... That is useful to know.

Assignee: nobody → ehsan
See Also: → 1556093

The classifications as "Intermittent" and the comments by the intermittent reporter get posted because it requires to either change the status from "intermittent" to "expected fail" for every single occurrence when it gets classified or wait until the simulation is done and then go through all the failures, collect them and classify them. Sorry about the confusion.

Not a problem, makes sense! Thanks for your explanation.

Here we are coming from LocalStorage::SetItem, notifying the cache observers, and we use the document URI not the principal to identify which origin the storage notification is for to the parent process here.

In response to that message we run LocalStorageCacheParent::RecvNotify. There we have two LocalStorageCacheParent objects, one that has a principal with mFirstPartyDomain empty and the other one set to "example.net" (the principals belonging to the iframe in the tracker tab and the main frame in the normal tab respectively). We ignore the first object because of the inequality check here and send a notification to the second object.

In response to that message we run LocalStorageCacheChild::RecvObserve. There aPrincipalInfo is "https://tracking.example.org^firstPartyDomain=example.net" (because it is coming from LocalStorageCacheParent::mPrincipalInfo in the paragraph above, see bug 1556082). Storage::NotifyChange will call StorageNotifierService::Broadcast where we iterate over the existing observers and try to pick the right one. Here we obviously have an observer from the iframe inside the normal tab (which has a principal with origin "https://tracking.example.org^firstPartyDomain=example.net"). We pick that one because of this check and send a storage event to that one, but that isn't expected, since now we've effectively ended up sending a storage event to the tab that originated the storage change by having called LocalStorage::SetItem in the first place.

This was an analysis of what is going on here. I am still unsure how to fix this. I think the key here is probably a combination of bug 1556082 and the fact that we don't look at the origin of the document that initiates the storage change anywhere here (see above where I pointed out we're recording the document URI not the principal.)

Depends on: 1556881, 1556082

baku, if you have any tips until I look at this again tomorrow that would be highly appreciated!

Flags: needinfo?(ehsan) → needinfo?(amarchesini)

I wrote a patch for bug 1556082. I think the problem is that we don't recreate the localStorage object when storage-access is granted. Because of that, the 2 windows have 2 localStorage objects with 2 different StoragePrincipal and they are not able to communicate properly.

Flags: needinfo?(amarchesini)

(In reply to Andrea Marchesini [:baku] from comment #6)

I wrote a patch for bug 1556082. I think the problem is that we don't recreate the localStorage object when storage-access is granted. Because of that, the 2 windows have 2 localStorage objects with 2 different StoragePrincipal and they are not able to communicate properly.

No. At no time in comment 4 was storage ever granted. :-) FWIW comment 4 was all from this subtest.

The underlying bug here is that LocalStorageCacheParent::RecvNotify doesn't perform sufficient filtering. There we are only careful to not call SendObserve() on our own actor, but we might happily call it on a cross-origin actor. That is what's happening here. See the second paragraph in comment 4 which shows how we receive the notification from an actor for "https://tracking.example.org^firstPartyDomain=example.net" and send it to an actor for "https://tracking.example.org".

Flags: needinfo?(ehsan)

(In reply to Oana Pop-Rus from comment #10)

The patch was applied on today beta simulation push and it caused build bustages on StorageIPC.cpp : https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=7aa7e7c3985c4f822a75deafdd17d82ebfb76e4d&selectedJob=250350308

Thanks, I fixed that before submitting the patch to autoland.

Flags: needinfo?(ehsan)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b4358c2ea5f
Only notify same-origin actors in LocalStorageCacheParent::RecvNotify(); r=baku
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Regressions: 1570644
You need to log in before you can comment on or make changes to this bug.