Closed Bug 818667 Opened 7 years ago Closed 7 years ago

Put HTTP cache in cookie jars and hook it up to private-data-clearing API

Categories

(Firefox OS Graveyard :: General, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

RESOLVED FIXED
B2G C3 (12dec-1jan)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: m1, Assigned: jdm)

References

Details

(Keywords: feature)

Attachments

(1 file, 2 obsolete files)

STR:
1.Open browser and load cnn.com or something
2.Put the device in airplane mode by long pressing power key and selecting APM.
3.Goto browser setttings and "Clear cookies and store data"
4. Refresh the page opened in #1

Expected result: 404 or some other error message.
Actual result: Page in #1 is successfully redrawn.

Using the task manager to exit the browser app, restarting it and navigating back to the same page in #1 also re-produces the page from cache.   Rebooting the device seems to flush the cache though.
I thought that setting doesn't clear the cache. It clears IndexedDB, localStorage, etc.

Ben, can you list what exactly is cleared?
blocking-basecamp: ? → +
Keywords: regression
Priority: -- → P2
Clear Private Data clears cookies, localStorage, IndexedDB and appcache stored by web content in the platform.

Clear History clears global history (which is stored by the browser app in IndexedDB) and now also clears session history from the platform.

I don't think clear private data clears the browser cache. In fact I only noticed the cache working last week!

It is quite bad that we don't provide any way to clear the cache. Jonas, do you know the reason for this?

Renominating based on the fact that I don't think this is a regression, we need to decide if adding a new feature to clear the browser cache is blocking-basecamp.
blocking-basecamp: + → ?
Keywords: regression
Priority: P2 → --
Should we create a new feature for that ?
Flags: needinfo?(jonas)
Flags: needinfo?(clee)
No longer blocks: 808607
The reason we don't so far is that we've only been looking at "personal data" and the cache if generally not considered as part of that.

Once bug 814247 is fixed it would be relatively easy to add an API for clearing the browser cache though.

Is clearing browser cache a hard requirement?
Depends on: 814247
Flags: needinfo?(jonas)
Yes, this is a v1 requirement.

BROWSER-072	As a user, I want to clear my private data by clicking the "Clear Private Data" button and clicking the "OK" button so I can better control what user data is being stored in the browser.
blocking-basecamp: ? → +
Flags: needinfo?(clee)
Target Milestone: --- → B2G C3 (12dec-1jan)
The platform part is already blocking, P1, C3. Assigning to Ben for now. Ben, any work required here, or is this fixed entirely by the platform bug?
Assignee: nobody → ben
Keywords: feature
Priority: -- → P1
Wait, the actual question wasn't actually answered.

We already support clearing private data. Both on the platform and in the UI.

The question here is specifically if clearing the cache is a requirement. Or if the cache is considered part of "private data".

I'm honestly not sure who would answer that question. Checking with Lucas if he has an answer or if he knows who should make this decision.
blocking-basecamp: + → ?
Flags: needinfo?(ladamski)
Summary: "Clear cookies and store data" doesn't seem to clear stored data. → "Clear cookies and store data" doesn't seem to clear cache.
The understanding was that cache was part of 'private data'.  If someone can confirm if this is available without any platform work, that would be great.  

Ben, do you have any background on this?
> If someone can confirm if this is available without any platform work, that would be 
> great.

I believe the platform work for this is not in place.
There's certainly platform work that needs doing to support this feature. Whether or not any Gaia work needs doing depends on whether clearing the cache uses a separate API call.

If it's separate, we also need input from UX on whether there should be a separate button for this or whether the Clear Private Data button should do both.

Note that it still won't be possible to manually clear this data for apps or homescreen bookmarks.
Flags: needinfo?(lco)
I agree that clearing the cache should be part of clearing private data.  I don't think there's a meaningful separation between the two workflows.
Flags: needinfo?(ladamski)
Michael, does this block any of your milestones?
Flags: needinfo?(mvines)
Jonas/Justin, do you think we should hook the same API call up to clearing cache too?

If this is the case there will be no necessary changes in Gaia.
Flags: needinfo?(lco) → needinfo?(jonas)
> Jonas/Justin, do you think we should hook the same API call up to clearing cache too?

Yes; I don't see why they should be different.
Flags: needinfo?(jonas)
(In reply to Daniel Coloma:dcoloma from comment #12)
> Michael, does this block any of your milestones?

No.  This issue was just raised because the reported behaviour was unexpected but doesn't fail any particular TC we run.
Flags: needinfo?(mvines)
Yeah, that's what we should do. Though we don't yet have per-application caches, so we might need to implement that first :(
Triage: We're not sure where this bug should live and it seems like it should go through B2G (platform) triage to get prioritised. Can someone find the right home for it?
Component: Gaia::Browser → General
QA Contact: nhirata.bugzilla
A few months ago we discussed what "clear private data" would do in B2G v1.  I remember coming to the conclusion that we'd accept just cookies and history for v1 but Kev can confirm.

We discussed this during triage and while it'd be nice to have, it seems like scope creep and thus would not be a blocker.
Flags: needinfo?(kev)
Clarifying what's needed here. And since Josh has been doing similar work for the auth cache recently lets see if he can take this one too.

Still would like to get an official word on if this is a product requirement or not.
Assignee: ben → josh
Summary: "Clear cookies and store data" doesn't seem to clear cache. → Put HTTP cache in cookie jars and hook it up to private-data-clearing API
Attached patch Put HTTP cache into app jars. (obsolete) — Splinter Review
This passes the tests I wrote. The one bit I don't like it the duplication of session names from nsHttpChannel to nsCacheService; do you have any good ideas for how to avoid that?
Attachment #693093 - Flags: review?(michal.novotny)
Note that the webapps-clear-data notification has somewhat unintuitive behavior for the aBrowserOnly flag:

If aBrowserOnly is *false*, we should clear all data for the provided appid
If aBrowserOnly is *true*, we should clear only the data for the appid where the browser flag is set to true.


We use aBrowserOnly=false to uninstall an app, which means that we want to get rid of all data associated with that app.

We use aBrowserOnly=true to clear private data for browser content associated with an app.
Appears that everyone we ask is saying that http-cache is private data. And it's how we're treating it in other products.
blocking-basecamp: ? → +
Flags: needinfo?(kev)
Comment on attachment 693093 [details] [diff] [review]
Put HTTP cache into app jars.

>      case nsICache::STORE_IN_MEMORY:
> -        return isPrivate ? "HTTP-memory-only-PB" : "HTTP-memory-only";
> +        sessionName.AssignASCII(isPrivate ? "HTTP-memory-only-PB" : "HTTP-memory-only");
> +        break;
>      case nsICache::STORE_OFFLINE:
> -        return "HTTP-offline";
> +        sessionName.AssignLiteral("HTTP-offline");
> +        break;
>      default:
> -        return "HTTP";
> -    }
> +        sessionName.AssignLiteral("HTTP");
> +        break;
> +    }
> +    sessionName.Append(':');
> +    sessionName.AppendInt(appId);
> +    sessionName.Append(':');
> +    sessionName.AppendInt(inBrowser);

This is wrong. First, the patch MUST NOT change the current session names in any way. This change is equivalent to clearing the whole cache since all old entries would be inaccessible.

Also, the session name shouldn't contain a colon. First colon divides session name and the key. So e.g. entry with the hash key "HTTP:0:0:http://www.foo.com/" would be accessible in session "HTTP" as entry with key "0:0:http://www.foo.com/".


> The one bit I don't like it the duplication of session names from
> nsHttpChannel to nsCacheService; do you have any good ideas for how to avoid
> that?

Move GetCacheSessionNameForStoragePolicy() to nsHttpHandler and let nsHttpHandler do the cleanup.
Attachment #693093 - Flags: review?(michal.novotny) → review-
FWIW, what we do elsewhere is to not add a prefix if appid=0 && browserflag=false
Attached patch Put HTTP cache into app jars. (obsolete) — Splinter Review
Attachment #695510 - Flags: review?(michal.novotny)
Attachment #693093 - Attachment is obsolete: true
Comment on attachment 695510 [details] [diff] [review]
Put HTTP cache into app jars.

>  #include "nsITimer.h"
>  
> -
>  #include "mozilla/net/NeckoCommon.h"

>      "suspend_process_notification",
> -    "resume_process_notification"
> +    "resume_process_notification",
>  };

Remove these changes from the patch.


> +/*static*/ void
> +nsCacheService::ClearCacheForSession(const char* aClientId)
> +{
> +    gService->EvictEntriesForClient(aClientId, nsICache::STORE_ANYWHERE);
> +}
> +

Use existing interface to evict the entries. I.e. call nsICacheSession::evictEntries() on appropriate cache session.


> +    nsCOMPtr<nsILoadContext> loadContext;
> +    NS_QueryNotificationCallbacks(this, loadContext);
> +    uint32_t appId = NECKO_NO_APP_ID;
> +    bool isInBrowser = false;
> +    if (loadContext) {
> +        loadContext->GetAppId(&appId);
> +        loadContext->GetIsInBrowserElement(&isInBrowser);
> +    }

The same code is used at two places, so it might worth to make a function from it.
Attachment #695510 - Flags: review?(michal.novotny) → review-
Attachment #696192 - Flags: review?(michal.novotny)
Attachment #695510 - Attachment is obsolete: true
Attachment #696192 - Flags: review?(michal.novotny) → review+
If this has issues again, was thinking perhaps it requires a clobber?
Nope, it's just that GCC disallows the template-based compile-time array length calculator for anonymous struct arrays defined inside of a function.
https://hg.mozilla.org/mozilla-central/rev/83c45c2f712b
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Duplicate of this bug: 792171
Blocks: 956930
No longer blocks: 956930
You need to log in before you can comment on or make changes to this bug.