1588887 - Ensure that tabs/iframes for origins that were open when data clearing APIs or UIs were used are not able to write data to storage after the clearing operation completes. This includes freshly created or navigated iframes.

Status: NEW

(Core :: Storage: Cache API, defect, P3)

Description

[Virginia (please send need info request to Brindusa Tot)]

Preconditions:
visit https://www.reddit.com/ and click "I agree" on the cookies banner at the bottom of the page, to have a considerable amount of cookies stored

Steps to reproduce:

1 Open Firefox and go to about:preferences, "Privacy & Security" section.

• Firefox is successfully opened.
• The about:preferences page is successfully loaded.
• The options for "Privacy & Security" section are displayed.

2 In the "Cookies and Site Data" set the "Delete cookies and site data when Firefox is closed".

• The option is selected.

3 Close Reddit or all the sites opened and reopen Firefox.

• All the sites are closed
• The browser opens without any issues.

4 Go to about:preferences#privacy -> Cookies and Site Data -> click on "Manage Data..."

• The "Manage Cookies and Site Data" subdialog is displayed.
• THERE ARE sites listed in the subdialog.

Note that this bug is opened due to https://bugzilla.mozilla.org/show_bug.cgi?id=1584986 having been fixed for Windows 10 and Linux. I was able to verify the issue was fixed on beta 70.0b14 (Build ID 20191010142853) for Windows 10 and Ubuntu 18.04. However, on Mac OSX10.14 the cookies for reddit.com were deleted only after I closed the Firefox 70.0b14 app twice:

1. I opened FF beta app, went to reddit.com
2. Went to about:preferences --> "Cookies and Site Data" set the "Delete cookies and site data when Firefox is closed".
3. Closed Reddit page.
4. quit the application from Firefox menu --> Quit Firefox.
5. Open Firefox Beta again, checked "Cookies and Site Data", Reddit cookies were still there.
6. Quit Firefox app again
7. Open Firefox, check "Cookies and Site Data", Reddit cookies were successfully deleted.

I tried this both by closing app from Firefox app menu--> Quit Firefox, and repeated the steps with a new profile, this time Force Quitting app from Dock, the results were the same, I had to quit Firefox 70b14 twice for Reddit cookies to be deleted.

Comment 1

[Johann Hofmann [:johannh]]

Thanks for filing the follow-up. Tom, do you have time to look into this?

Comment 2

[Tom Tung [:tt, :ttung]]

Hmm, so, I tested it on a disable-opt build for five times and I couldn't reproduce it for a Beta build on my Mac.

However, I was able to reproduce it with a opt build for the beta-build on my Mac. And, I logged information for that since I couldn't run it with a debugger(IIUC, if I want to set a breakpoint, I need a disable-opt build).

Through logging, I pretty sure it ran into here [1] and succeeded. I also check the leafName of the file object in [1] and it's "https+++www.reddit.com".

So, in short, the listOrigins did collect the origin for "https://www.reddit.com" and clearOrigin did try to remove the origin directory. I suspect there is an issue inside [2].

To elaborate more:
The way I reproduce was:

• Open Firefox
• Open a tab and navigate it to the website reddit
• Check the origin directory ("https+++www.reddit.com") is truly created in the profile directory
• Click the option on the about:preference
• Close the tab for reddit
• Quit the Firefox
• Check whether the origin directory ("https+++www.reddit.com") is removed

[1] https://searchfox.org/mozilla-central/rev/171109434c6f2fe086af3b2322839b346a112a99/dom/quota/ActorsParent.cpp#9475-9477
[2] https://searchfox.org/mozilla-central/rev/171109434c6f2fe086af3b2322839b346a112a99/xpcom/io/nsLocalFileUnix.cpp#1071

Comment 3

[Tom Tung [:tt, :ttung]]

In our team meeting, Jan and Andrew mentioned it might be possible in two conditions

• DOM Cache might access the file while deleting
• DOM Cache or ServiceWorker reconstruct the file right after the deletion.

I only suspected it the first condition. I will check/verify this.

Comment 4

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt, :ttung] from comment #3)

• DOM Cache or ServiceWorker reconstruct the file right after the deletion.

I verified it's this by setting a breakpoint on https://searchfox.org/mozilla-central/rev/171109434c6f2fe086af3b2322839b346a112a99/dom/quota/ActorsParent.cpp#9475-9477.

While the debugger hit the breakpoint, the directory was removed. And then the directory was constructed by DOM cache (probably triggered by the ServiceWorker). So it's probably the same issue as bug 1589708 since the second try in STR succeeded (SW hasn't been reconstructed).

So, there is no privacy issue since the quota storage has already removed. The result of the STR in comment #0 is an empty DOM Cache storage which only has an empty SQLite Database and an empty morgue directory inside. But, we still need to fix this to avoid confusion for users.

Comment 5

[Tom Tung [:tt, :ttung]]

Moving bug 1589708 to "Depends on" since I suspect this is a duplicate bug for that. I will verify the result once that bug is fixed.

Comment 6

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

(In reply to Tom Tung [:tt, :ttung] from comment #4)

So, there is no privacy issue since the quota storage has already removed. The result of the STR in comment #0 is an empty DOM Cache storage which only has an empty SQLite Database and an empty morgue directory inside. But, we still need to fix this to avoid confusion for users.

Just to clarify for anyone following along to the bug, there are 2 aspects to the privacy threat model here:

1. The origin of the site being visible on disk or via Firefox's UI.
2. The contents of what the site was storing.

Tom is indicating that our failure here is point 1, not point 2. Failing to clear point 1 is still a privacy problem and I am moving to address bug 1589708 today.

Comment 7

[Tom Tung [:tt, :ttung]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #6)

(In reply to Tom Tung [:tt, :ttung] from comment #4)

So, there is no privacy issue since the quota storage has already removed. The result of the STR in comment #0 is an empty DOM Cache storage which only has an empty SQLite Database and an empty morgue directory inside. But, we still need to fix this to avoid confusion for users.

Just to clarify for anyone following along to the bug, there are 2 aspects to the privacy threat model here:

1. The origin of the site being visible on disk or via Firefox's UI.
2. The contents of what the site was storing.

Tom is indicating that our failure here is point 1, not point 2. Failing to clear point 1 is still a privacy problem and I am moving to address bug 1589708 today.

Yes, I meant the content is not leaking any information but the origin name still. Thanks for the correction!

Comment 8

[Tom Tung [:tt, :ttung]]

From our team meeting, we suspect this might be not the same issue as bug 1589708 since bug 1589708 is on Nightly and this is on the Beta.

So the next step of this for me is to figure out where the request is coming from. (I guess mostly is to ensure it's not related to SW since it should have already been removed before we are in this step)

Jan mentioned that it might be related to some test failures for ClearSiteData and it's probably a long-standing issue. And it could be DOM Cache operation lose for accessing while completing directory access from QM::ResetOrClearOp since ResetOrClearOp has an exclusive directory lock. So that it access the directory right after that. Besides, he pointed out we could consider deferring the access for the SQLite database for DOM Cache since it's slow to do that (but it needs a bigger refactoring).

Andrew mentioned we might want to have an operation on QM/QMS to abort existing operations in this case (shutting down).

We might want to have more discussion on this to catch similar issues and find a proper way to fix this.

Andrew and Jan, please feel free to elaborate more if I missed anything somehow (I do think I miss some points). Thanks!

Comment 9

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt, :ttung] from comment #8)

So the next step of this for me is to figure out where the request is coming from. (I guess mostly is to ensure it's not related to SW since it should have already been removed before we are in this step)

I will schedule a meeting when I finish the investigation for this.

Comment 10

[Tom Tung [:tt, :ttung]]

We need to figure out the reason why there is another open directory request from DOM Cache to QuotaManager right after the deletion. I will work on this next year.

Unassign myself since I don't have time to work on this before the end of this year. Everyone is welcome to take it.

Comment 11

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt, :ttung] from comment #2)

The way I reproduce was:

• Open Firefox
• Open a tab and navigate it to the website reddit
• Check the origin directory ("https+++www.reddit.com") is truly created in the profile directory
• Click the option on the about:preference
• Close the tab for reddit
• Quit the Firefox
• Check whether the origin directory ("https+++www.reddit.com") is removed

I can still reproduce this issue if I follow the steps above.

However, I find out that the cache directory is created when I close the tab for that. If I close the tab and then remove the storage, then the issue disappears.

The storage that was created when the tab was closed:

• DOM Cache
• LocalStorage

So, the question that coming up is whether it's okay to allow a website to register a service worker or use quota storage in the session after their storage is removed by about:preference.

I suspect the website (Reddit) stores some storage to remember the status when it gets the event for closing.

If it's okay to do that, then I think we can close this bug because the storage is not restored after the tab is close and then its storage is removed. (I think the original issue which I was worried about is that the storage is restored right after the removal because another open directory request is queued)

If it's not, I wonder if we need to do something like:

• QM sends a notification to let existing session in the child process for the removed storage knows that their storage is removed and they shouldn't request further storage in the session.

Personally, I lean to that it's okay to use storage in the session even after their storage is removed, but I'm not so sure if it's correct behavior.

Jan, and Andrew, what do you think?

Comment 12

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

I think this is a question for :johannh. And needinfo-ing :baku for implementation complexities and cc'ing :annevk for spec-related thinkin':

:johannh, what should we be doing in situations where:

1. The user explicitly clears storage for an origin.
2. The origin is still open in a tab.

The "forget the last TIME INTERVAL" UI explicitly closes tabs/windows, but about:preferences does not.

It seems like we'd want to do something dramatic like using the Clients API to:

• Navigate all windows to a special new UI page like about:cleared?origin=FOO&url=BAR that says something like "You cleared the data for FOO so we've replaced the page BAR with this placeholder. If you want to reload the page, push this button, but know that the site can still use storage with current permissions <whatever>".
• Probably tell the RemoteWorkerService to block creation of new workers for the given origins. (As opposed to doing something specialized for ServiceWorkers and SharedWorkers at this stage.)
• Wait for all the globals for impacted origins to go away.
• Only then wipe storage.

Note that this is conceptually different from the clear-site-data use-case as the user is in control, not the site.

Comment 13

[Johann Hofmann [:johannh]]

So, are we 100% sure that's what is going on here? The user is clearing their data but has the tab still open?

If yes, then I would generally not worry about this too much and I would definitely not consider platform fixes to mitigate this. We could file a UX suggestion bug to show a dialog that says

You want to clear all data for example.com, which is still open in 5 tabs. To ensure no additional data is added, you need to close these tabs now.

[Close tabs and proceed] [Proceed without closing]

(copy obviously up to bikeshed and UX)

Comment 14

[Anne (:annevk)]

I quite like the idea of the placeholder tab, but I can also see warning them about currently opened tabs as they might want to complete something.

"You cleared site data for example.com. [Navigate to example.com/page again.]"

Either way, it seems there would be some tricky cases with third-parties. If example.com embeds test.example and test.example has non-partitioned storage and the user then wants to clear test.example. What should the dialog say in order to make it clear to the user what will happen?

Comment 15

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

(In reply to Johann Hofmann [:johannh] from comment #13)

So, are we 100% sure that's what is going on here? The user is clearing their data but has the tab still open?

This is what we're discussing for Tom's specific reproduction, but the original issue for this bug was that of the automated clearing at shutdown. These situations potentially overlap depending on what we want to do in the interactive case where tabs are open, as that is a more complex barrier situation.

You want to clear all data for example.com, which is still open in 5 tabs. To ensure no additional data is added, you need to close these tabs now.

[Close tabs and proceed] [Proceed without closing]

Is there a specific use-case and associated threat model we're assigning to these two choices? "[Proceed without closing]" would seem to provide no privacy guarantees whatsoever. "[Clear data for these origins when I close Firefox]" might work better if the intent is to enable the user to express their desire to wipe the data but they want to keep using the tabs, albeit adding additional UX complexity.

As a separate concern, how would this interact with iframes? Would all (top-level) tabs that contained any iframes subject to closure be closed too? Or would the iframes be about:blanked or about:cleared?

Tom, as far as this bug goes, I think we might want to build on the marionette test Yaron wrote that lives at https://searchfox.org/mozilla-central/source/toolkit/components/cleardata/tests/marionette/test_service_worker_at_shutdown.py to get specific coverage for the original issue for this bug. Specifically, we'd want to validate the domains no longer show up in the quota usage after a restart. This might also want the ServiceWorker and/or page to take additional steps to attempt to generate late writes at shutdown. The most practical solution might be to run Firefox with MOZ_LOG=mozStorage:5 and browse to reddit and see what writes it's inducing, then figure out how it's initiating those writes, and making sure we reproduce them. For example, it looks like reddit adds 2 "beforeunload" handlers, which it might be using to do some kind of beacon/metrics thing that end up using storage and effectively racing shutdown, especially if it postMessages the ServiceWorker or something. (Unfortunately, the listeners' actions are somewhat obfuscated by the use of @sentry to wrap everything in extra error handling that gets reported, which perhaps might have its own storage side-effects?)

Comment 16

[Tom Tung [:tt, :ttung]]

needinfo myself to write a test for ensuring the behavior.

Comment 17

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

(NI for comment 15)

Comment 18

[Johann Hofmann [:johannh]]

Sorry for the delay,

This is what we're discussing for Tom's specific reproduction, but the original issue for this bug was that of the automated clearing at shutdown. These situations potentially overlap depending on what we want to do in the interactive case where tabs are open, as that is a more complex barrier situation.

Does it make sense to file a new bug for the open-tab issue? I feel like we should already have a bug for this...

As a separate concern, how would this interact with iframes? Would all (top-level) tabs that contained any iframes subject to closure be closed too? Or would the iframes be about:blanked or about:cleared?

I imagined the former, we could list them out? I think having something like about:cleared is also fine but I would not scope it at frame-level, since the breakage would be too subtle for users to understand IMO.

Not sure if there's other input you need from me :)

about:cleared or similar sounds like a nice student project though...

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

For more information, please visit auto_nag documentation.

Comment 20

[Jens Stutte [:jstutte]]

:asuth, I think if we want to move this forward we will need to restate what is the expected scope of this bug.

Comment 21

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

I discussed this scenario in greater detail and with a successor proposal in https://bugzilla.mozilla.org/show_bug.cgi?id=1713139#c9. I think the current best approach to this is to address this during implementation of our StorageKey abstraction covered by bug 1776271 and I've commented at https://bugzilla.mozilla.org/show_bug.cgi?id=1776271#c5 specifically about this use case.

This bug should likely remain open even if we also create a QuotaManager tracking bug, as this is a problem that could be perceived individually by different storage APIs.

Comment 22

[Yalena Maldonado]

Attachment: zygatopanch19746553.txt

Thanks from ticageto1975.