Closed Bug 1556093 Opened 4 months ago Closed 4 months ago

Perma browser_partitionedLocalStorage_events.js | Undefined value! - "tracker-0.3787650295256073" == "undefined" - | localStorage data was cleared - got true, expected false - when Gecko 69 merges to Beta on 2019-07-01

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
thunderbird_esr60 --- unaffected
firefox-esr60 --- unaffected
firefox67 --- unaffected
firefox67.0.1 --- unaffected
firefox68 --- unaffected
firefox69 + fixed

People

(Reporter: CosminS, Assigned: ehsan)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

Depends on: 1555562

There are at least two problems happening here, perhaps even more.

One is bug 1555562 which Nihanth is working on. The other is that here https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/dom/storage/LocalStorageManager.cpp#245 we use aPrincipal rather than aStoragePrincipal and this is used to construct a PBackgroundLocalStorageCache actor but with the wrong principal, and later on this actor will send a storage change notification back to the content process which will have the wrong principal attached to it: https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/dom/storage/StorageIPC.cpp#84

Perhaps there would be more to fix after fixing these...

Flags: needinfo?(ehsan)
Flags: needinfo?(ehsan)

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

This fail still appears on late beta sims push: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=c2d8fe0f14155a85e80e13e21bee8867356ca027&searchStr=mochitest&selectedJob=249920994

Yes, there is no need for updates here, thanks! I will post a comment when the bug is fixed so that you can verify that.

Another example of failure from today: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=c2d8fe0f14155a85e80e13e21bee8867356ca027&selectedJob=249922446&searchStr=a783c28f27c68d74aa877554b357a28b3f30b46b

That failure (and also whatever comment 1 and comment 2 are pointing to) have nothing to do with this bug whatsoever and should be filed separately. (I haven't even looked at it.)

Flags: needinfo?(ehsan)
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
Keywords: leave-open
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/52ffeb5aaab2
Use the storage principal when constructing a PBackgroundLocalStorageCache actor; r=baku
See Also: → 1556812

(In reply to :Ehsan Akhgari from comment #3)

There are at least two problems happening here, perhaps even more.

One is bug 1555562 which Nihanth is working on. The other is that here https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/dom/storage/LocalStorageManager.cpp#245 we use aPrincipal rather than aStoragePrincipal and this is used to construct a PBackgroundLocalStorageCache actor but with the wrong principal, and later on this actor will send a storage change notification back to the content process which will have the wrong principal attached to it: https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/dom/storage/StorageIPC.cpp#84

Perhaps there would be more to fix after fixing these...

The only two parts needed here are these which are both on autoland right now. After those fixes, bug 1556812 is still left which I will look into next.

Flags: needinfo?(ehsan)
Keywords: leave-open
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Target Milestone: mozilla69 → ---
Summary: Perma browser_partitionedLocalStorage_events.js/test_ext_storage_cleanup.html | Undefined value! - "tracker-0.3787650295256073" == "undefined" - | localStorage data was cleared - got true, expected false - when Gecko 69 merges to Beta on 2019-07-01 → Perma test_ext_storage_cleanup.html | Undefined value! - "tracker-0.3787650295256073" == "undefined" - | localStorage data was cleared - got true, expected false - when Gecko 69 merges to Beta on 2019-07-01

(In reply to Alexandru Michis [:malexandru] from comment #11)

Still happening in today's beta sim: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=adc615b23d058a356988dab630b01fc87cc3a438&searchStr=%285%29&selectedJob=250164282

Two questions:

  1. Why is this changeset included in that try push? It hasn't landed yet, and it was never tested by me. I don't know what its impact on this test is...

  2. Did that push include the fix to bug 1555562?

Since I cannot reproduce this bug any more, the reason why you are seeing it most likely goes back to one of these...

Flags: needinfo?(malexandru)

(In reply to :Ehsan Akhgari from comment #12)

(In reply to Alexandru Michis [:malexandru] from comment #11)

Still happening in today's beta sim: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=adc615b23d058a356988dab630b01fc87cc3a438&searchStr=%285%29&selectedJob=250164282

Two questions:

  1. Why is this changeset included in that try push? It hasn't landed yet, and it was never tested by me. I don't know what its impact on this test is...

FWIW I just tested this changeset locally and it has no impact on whether this test passes or fails thankfully, so this is probably not the cause.

Hmm, https://hg.mozilla.org/try/file/adc615b23d058a356988dab630b01fc87cc3a438/dom/interfaces/storage/nsIDOMStorageManager.idl shows that the push also had the fix for bug 1555562...

And then I looked more carefully at the try push and I saw that the failures are in a whole different test: test_ext_storage_cleanup.html which I don't know anything about. Could you maybe file a separate bug on that and CC the right people who have in the past changed that test? Thanks!

Status: REOPENED → RESOLVED
Closed: 4 months ago4 months ago
Resolution: --- → FIXED

Ehsan, hello. The initial failures here are in test_ext_storage_cleanup.html as the bug was filed: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=249377829&repo=try&lineNumber=5160

Sorry if I did not understand your last comment. Could you please clarify?

Flags: needinfo?(ehsan)
Flags: needinfo?(malexandru)

(And to be more clear about what happened in this bug, the original failures were in both browser_partitionedLocalStorage_events.js which I know about and test_ext_storage_cleanup.html which I know nothing about and it seems like now the former is fixed and we only have the latter left to fix, I was suggesting to move that to another bug since that's probably unrelated to this.)

(In reply to Andreea Pavel [:apavel] from comment #15)

Ehsan, hello. The initial failures here are in test_ext_storage_cleanup.html as the bug was filed: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=249377829&repo=try&lineNumber=5160

Sorry if I did not understand your last comment. Could you please clarify?

Sorry I mid-aired, comment 16 is explaining what happened here. :-) It is probably worth bisecting that failure separately too (I haven't really investigated it at all.)

Flags: needinfo?(ehsan)
No longer blocks: 1557122
Summary: Perma test_ext_storage_cleanup.html | Undefined value! - "tracker-0.3787650295256073" == "undefined" - | localStorage data was cleared - got true, expected false - when Gecko 69 merges to Beta on 2019-07-01 → Perma browser_partitionedLocalStorage_events.js | Undefined value! - "tracker-0.3787650295256073" == "undefined" - | localStorage data was cleared - got true, expected false - when Gecko 69 merges to Beta on 2019-07-01
You need to log in before you can comment on or make changes to this bug.