892488 - Stop prompting when websites use appcache

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

flip the pref (offline-apps.allow_by_default)

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 775875 [details] [diff] [review]
v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/111e2f8f0d68

Comment 3

[Honza Bambas (:mayhemer)]

Backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/7f177c80f3b9 since we have tests that check the prompt :))

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

How is this different than bug 648064? Have the issues in bug 648064 comment 13 been addressed?

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:briansmith), was [e-mail] (:bsmith) from comment #4)
> How is this different than bug 648064? Have the issues in bug 648064 comment
> 13 been addressed?

Has not.  But we still want to do it.  We want to see what impact this is going to have on using appcache by web sites.  This is just a pref flip.  We can always revert it back easily, even on release.

Comment 6

[Honza Bambas (:mayhemer)]

This fix won't be that simple as just flipping the pref.  

I just realized one thing: we were always wondering why synchronous main thread access to offline application sqlite database didn't cause any regression or main thread jank.  The reason was simple - the offline cache was not being checked on until the user allowed permission for it.  Flipping the pref gives this permission automatically, so with this patch we would now check appcache database for every top level load.  That is unacceptable.

I have to work more on this patch.

Comment 7

[(no longer active)]

(In reply to comment #6)
> This fix won't be that simple as just flipping the pref.  
> 
> I just realized one thing: we were always wondering why synchronous main thread
> access to offline application sqlite database didn't cause any regression or
> main thread jank.  The reason was simple - the offline cache was not being
> checked on until the user allowed permission for it.  Flipping the pref gives
> this permission automatically, so with this patch we would now check appcache
> database for every top level load.  That is unacceptable.

That's terrible.  :(

Comment 8

[Honza Bambas (:mayhemer)]

I think I have a patch that does what we want and doesn't cause regressions or unnecessary main thread I/O.  I have to make the tests work now and give it for review.

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v2

So, to write the patch to fix this bug properly w/o regression took few minutes.  But to fix our absolutely crappy offline tests and crappy offline/dom code took several hours!  

Before this change offline cache mochitests were actually running in a separate window.

Now tests are running directly in the test iframe.  It uncoverd a lot of problems and I needed to fix them all.

What the patch does:
- when offline-apps.allow_by_default is true and we hit a web page with a manifest, we automatically grant the offline-app privilege to the origin during the cache selection algorithm (call to new nsContentUtils::MaybeAllowOfflineAppByDefault helper form nsContentSink)
- it means that we don't check appcache on every load but still only for loads from domains that have this privilege set (no regression, yay!) ; before this patch offline-apps.allow_by_default at true value made all sites appear as having offline-app privilege and that would be causing load time regression
- some tests revert the setting to false for time of its execution (simplest solution)
- DOMOfflineResourceList (a.k.a window.applicationCache js object) had a bug:
  - appcache of the document were incorrectly replaced under some conditions
  - when there were an update in background (e.g. invoked by some other page) and ended with "no update" applicationCache was in bad state since mAvailableApplicationCache was identical to current appcache associated with the document ; hence state was not returning IDLE but was throwing
  - swapCache didn't behave well, it was able to delete current cache when called after no update or an error update ; it only must exchange appcache when available or delete it when last update returned OBSOLETE state

I think I've fixed few intermittent failures of ajax/offline tests.  I've also fixed long execution of test_updatingManifest.html (from 20 or more secs to a seconds or so).

Finally not a simple patch, but useful!


https://tbpl.mozilla.org/?tree=Try&rev=0eadf9e1662c

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 778141 [details] [diff] [review]
v2

Try run is green.  Requesting review.  For explanations on the patch please see comment 9.

Comment 11

[(no longer active)]

Comment on attachment 778141 [details] [diff] [review]
v2

Review of attachment 778141 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mostly good, requesting review from blassey on the java changes, and from sicking on nsDOMOfflineResourceList.cpp changes.

::: browser/base/content/test/test_offline_gzip.html
@@ +43,5 @@
> +  },
> +  reset: function() {
> +    this.prefBranch.setBoolPref("offline-apps.allow_by_default", this.defaultValue);
> +  }
> +}

It would probably be a good idea to put this object in a shared .js file and include it in both places.

::: content/base/src/nsContentUtils.cpp
@@ +1420,5 @@
> +  rv = permissionManager->AddFromPrincipal(
> +    aPrincipal, "offline-app", nsIPermissionManager::ALLOW_ACTION,
> +    nsIPermissionManager::EXPIRE_NEVER, 0);
> +  if (NS_FAILED(rv))
> +    return false;

It kind of bothers me that the other side of this code is buried inside nsOfflineCacheUpdateService.cpp.  I think it would be a good idea to maybe move the OfflineAppPermForURI function there to nsContentUtils next to this method.  But we can do that in a follow-up bug.

::: dom/src/offline/nsDOMOfflineResourceList.cpp
@@ -83,5 @@
>    : mInitialized(false)
>    , mManifestURI(aManifestURI)
>    , mDocumentURI(aDocumentURI)
>    , mExposeCacheUpdateStatus(true)
> -  , mDontSetDocumentCache(false)

I'm not 100% sure if I understand the changes in this file...

::: dom/tests/mochitest/ajax/offline/obsolete.html
@@ +18,5 @@
>                       "Status should be 5 (obsolete)");
>  
>      // Now swapCache(), and our new status should be UNCACHED.
> +    try{
> +    applicationCache.swapCache();}catch(e){dump(e)}

Do you expect this to throw in practice?

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 778141 [details] [diff] [review]
v2

Review of attachment 778141 [details] [diff] [review]:
-----------------------------------------------------------------

only java change is to testDoorHanger.java.in, which Margaret wrote. Passing the review to her.

Comment 13

[:Margaret Leibovic]

Comment on attachment 778141 [details] [diff] [review]
v2

Review of attachment 778141 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on the testDoorHanger part.

::: mobile/android/base/tests/testDoorHanger.java.in
@@ +1,5 @@
>  #filter substitution
>  package @ANDROID_PACKAGE_NAME@.tests;
>  
> +import @ANDROID_PACKAGE_NAME@.*;
> + 

Nit: whitespace.

@@ +76,5 @@
>          mAsserter.is(mSolo.searchText(GEO_MESSAGE), false, "Geolocation doorhanger notification is hidden when opening a new tab");
>  
> +
> +        boolean offlineAllowedByDefault = true;
> +        try { 

Nit: Trailing whitespace.

@@ +80,5 @@
> +        try { 
> +            // Save offline-allow-by-default preferences first
> +            JSONArray getPrefData = new JSONArray();
> +            getPrefData.put("offline-apps.allow_by_default");
> +            JSONObject message = new JSONObject();

Nit: Put a blank line above this line to make this a bit more readable.

@@ +95,5 @@
> +            while (!requestId.equals("testDoorHanger")) {
> +                data = new JSONObject(eventExpecter.blockForEventData());
> +                requestId = data.getString("requestId");
> +            }
> +            eventExpecter.unregisterListener();

Mmm, copy/paste :) I'm going to file follow-up bug for us to split this pattern out into some helper method somewhere, since we do this all over the place.

@@ +108,5 @@
> +            JSONObject jsonPref = new JSONObject();
> +            jsonPref.put("name", "offline-apps.allow_by_default");
> +            jsonPref.put("type", "bool");
> +            jsonPref.put("value", false);
> +            mActions.sendGeckoEvent("Preferences:Set", jsonPref.toString());

I was wondering if we need to add something here to wait to make sure the pref is set, since we do that in some other tests:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testPasswordEncrypt.java.in#128
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testAddonManager.java.in#61

However, I see this is green on try, so it's fine as-is.

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I am totally against doing this if it means that it will make it trivial for any site that the user visits to fill on their hard disk. AppCache is not important enough to warrant that risk to users. This is high risk with virtually no reward. This prompt is not the only reason that nobody is using AppCache. We would be better of just dropping all AppCache support.

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #11)

Thanks.

> It kind of bothers me that the other side of this code is buried inside
> nsOfflineCacheUpdateService.cpp.  I think it would be a good idea to maybe
> move the OfflineAppPermForURI function there to nsContentUtils next to this
> method.  But we can do that in a follow-up bug.

I think this needs to stay as is, since the check needs to be exposed to JS.  nsContentUtils are just C++ thing.

> 
> ::: dom/src/offline/nsDOMOfflineResourceList.cpp
> @@ -83,5 @@
> >    : mInitialized(false)
> >    , mManifestURI(aManifestURI)
> >    , mDocumentURI(aDocumentURI)
> >    , mExposeCacheUpdateStatus(true)
> > -  , mDontSetDocumentCache(false)
> 
> I'm not 100% sure if I understand the changes in this file...

So, to explain: mDontSetDocumentCache when true prevents the document this nsDOMOfflineResourceList instance (a.k.a window.applicationCache) is bound to from being associated a new appcache version created by an update.  I've introduced that flag to prevent appcache to switch in the document when a script in that document called applicationCache.update().

Allowing test to run in the global test window shows that we needed to prevent document's appcache update from ANY update, regardless it had been invoked by applicationCache.update() from inside the document or not.

So, I've changed the code to not allow changing appcache of a document when that document already has a cache.

> 
> ::: dom/tests/mochitest/ajax/offline/obsolete.html
> @@ +18,5 @@
> >                       "Status should be 5 (obsolete)");
> >  
> >      // Now swapCache(), and our new status should be UNCACHED.
> > +    try{
> > +    applicationCache.swapCache();}catch(e){dump(e)}
> 
> Do you expect this to throw in practice?

Forgot to remove this.

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:briansmith), was [e-mail] (:bsmith) from comment #14)
> I am totally against doing this if it means that it will make it trivial for
> any site that the user visits to fill on their hard disk. AppCache is not
> important enough to warrant that risk to users. This is high risk with
> virtually no reward. This prompt is not the only reason that nobody is using
> AppCache. We would be better of just dropping all AppCache support.

This is an experiment.  I am quit curious what effect this is going to have.  

We know there is the risk and we know we should have some appcache eviction mech.  But we don't and I don't think we will soon have one, since only small effort is put in direction of appcache support.  Closed circle: we screwed the appcache implementation by having a prompt, so it's not widely used and there are UA filters that exclude Firefox from allowing appcache be used to not let the prompt pop-up, so we don't care to improve because no one is using appcache in Firefox.

I want to break this circle.

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Honza Bambas (:mayhemer) from comment #16)
> (In reply to Brian Smith (:briansmith), was [e-mail] (:bsmith)
> from comment #14)
> > I am totally against doing this if it means that it will make it trivial for
> > any site that the user visits to fill on their hard disk. AppCache is not
> > important enough to warrant that risk to users. This is high risk with
> > virtually no reward. This prompt is not the only reason that nobody is using
> > AppCache. We would be better of just dropping all AppCache support.
> 
> This is an experiment.  I am quit curious what effect this is going to have.

I think everybody is. That doesn't mean it is OK to land it in its current state.

> We know there is the risk and we know we should have some appcache eviction
> mech.  But we don't and I don't think we will soon have one, since only
> small effort is put in direction of appcache support.  Closed circle: we
> screwed the appcache implementation by having a prompt, so it's not widely
> used and there are UA filters that exclude Firefox from allowing appcache be
> used to not let the prompt pop-up, so we don't care to improve because no
> one is using appcache in Firefox.
> 
> I want to break this circle.

There are two acceptable ways to break the cycle: (a) drop AppCache, or (b) fix the eviction bug.

We shouldn't be landing changes like this on Nightly. People, especially people at Mozilla, use Nightly as their default browser. We should not be intentionally exposing them to totally unnecessary risks, even if we intend on undoing those risks in the release channel.

Comment 18

[Doug Turner (:dougt)]

bsmith, your point is taken.  the risk is known -- and these fill-disk problems exist today in other brosers.  we will do one of the things you mentioned to break the cycle.  don't think you need to block one on the other.  we should just make damn sure we fix it before the m-a uplift.

(yes, i am treating nightly like a pre-alpha)

Comment 19

[Honza Bambas (:mayhemer)]

Attachment: v2.1

I've only addressed the review comments and made some small cleanup.

try: https://tbpl.mozilla.org/?tree=Try&rev=198ecda04ac7

Comment 20

[Dão Gottwald [:dao]]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> Backed out
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7f177c80f3b9 since we
> have tests that check the prompt :))

Can we remove the pref and the prompt after we've shipped this?

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Dão Gottwald [:dao] from comment #20)
> (In reply to Honza Bambas (:mayhemer) from comment #3)
> > Backed out
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/7f177c80f3b9 since we
> > have tests that check the prompt :))
> 
> Can we remove the pref and the prompt after we've shipped this?

Potentially, but since there is some kind of a risk, I'd leave it at least one or two cycles.

Comment 22

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Doug Turner (:dougt) from comment #18)
> bsmith, your point is taken.  the risk is known -- and these fill-disk
> problems exist today in other browsers.

They are fixing them. And, 

> we will do one of the things you
> mentioned to break the cycle.  don't think you need to block one on the
> other.  we should just make damn sure we fix it before the m-a uplift.

"We" as in who? We have so many more important things to do in Necko than dick around with worse-than-useless and never-used AppCache. It would be sad to delay the landing of the HTTP cache fixes that affect *every single request* we make to make these AppCache changes that affect *virtually no requests* we make. Similarly, there are some any other important improvements to be make that affect huge percentages of requests.

Also, let's not forget that, due to extra disk access and extra trips through the event loop, in many ways AppCache is a performance *loss* with our current implementation. So, even if there weren't security issues with killing the prompt, enabling AppCache more often (not that any websites ever use it anyway) would still likely be a net loss for users. And, that's not going to change anytime soon, if we prioritize work correctly. And, even if it those performance issues weren't there, there's STILL the fact that nobody's legitimately using AppCache anyway so there's no upside to the additional risk that users are taking on.

> (yes, i am treating nightly like a pre-alpha)

Obviously, Nightly doesn't have the same level of quality control as Aurora and Beta and Release, but it must not be intentionally and deliberately made more prone to data loss and other security problems. We want more people to be testing Nightly, and not just people who have nothing to lose.

To put things in perspective, the Firefox team has had its Australis landing blocked for over four weeks due to a few very small performance regressions in Talos, because we don't tolerate even these minor performance regressions on Nightly. I would hope that it would go without saying that a we'd have a similar (at least) lack of tolerance of known security regressions on Nightly.

Comment 23

[Doug Turner (:dougt)]

> We have so many more important things to do in Necko than dick around with worse-than-useless and never-used AppCache.

Don't bring up prioritization in a tech bug report.  Totally orthogonal.  Serious issues, but not for this bug.

I do agree that AppCache is not interesting at all and we should stop investing.  We should probably remove the code too.  The data says it isn't used.  We added telemetry to get a better view.

The thinking was that we'd start collecting data.  Then remove the prompt to be able to determine if the prompt was a reason why people didn't use it.  I think that was a bump for adoption on indexed db.  sicking can tell you more.

I wasn't a champion for the removing the prompt.  I don't think I care if we do it or not -- but I think being blocked on this experiment is dumb.  We have better things to do (as you say) than to worry about appcache.  How's that review queue bsmith? :)

Sicking, you were at this meeting -- do you have anything to add?

Comment 24

[(no longer active)]

We are not exposing people to any *security* risks.  We're exposing them to *potential* diskfill types of attacks which already exist in every major browser out there.  Also, quite to the contrary of what Brian suggests, we land experimental things that can (and do) have sg-crit bugs in them on trunk all the time.  Known security risk is not always good enough of a reason to not land something.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 780537 [details] [diff] [review]
v2.1

Review of attachment 780537 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't check the test changes very thoroughly. If you need me to let me know.

::: content/base/src/nsContentUtils.cpp
@@ +1409,5 @@
> +  if (NS_FAILED(rv))
> +    return false;
> +
> +  if (!allowedByDefault)
> +    return false;

Maybe combine these two into a single if-statement.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think before we can say that it's not worth investing in appcache we need to have data to back that up.

Getting useful data means removing the prompt I believe.

It was thought that removing the prompt would be an easy task. Unfortunately it was tricker than expected.

I also don't really agree that we should stop investing in appcache. While I agree that it's important to improve performance in necko, I don't think that means that we can completely stop investing in offline.

From an enduser's point of view, having a website load X% faster is great. However if the website isn't available at all because you are offline, then it doesn't matter at all how fast it would load had you been online.

I wish as much as anyone else here that our appcache implementation was better, but it is what it is and we have to live with that.

Comment 27

[Florian Bender]

How much effort would it be to implement a quota prompt like for IndexedDB where it asks if the site is allowed to store >50MB? 

Or would you rather prefer having a (easier to implement) hard limit which just fails appcache if it uses too much space?

Comment 28

[Dão Gottwald [:dao]]

(In reply to Honza Bambas (:mayhemer) from comment #21)
> (In reply to Dão Gottwald [:dao] from comment #20)
> > (In reply to Honza Bambas (:mayhemer) from comment #3)
> > > Backed out
> > > https://hg.mozilla.org/integration/mozilla-inbound/rev/7f177c80f3b9 since we
> > > have tests that check the prompt :))
> > 
> > Can we remove the pref and the prompt after we've shipped this?
> 
> Potentially, but since there is some kind of a risk, I'd leave it at least
> one or two cycles.

Can you file a bug on this so we don't lose track of it?

Comment 29

[Honza Bambas (:mayhemer)]

Attachment: v2.2 [landed/backed out]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e91392042137

https://tbpl.mozilla.org/?tree=Try&rev=6fe3308c43ac
https://tbpl.mozilla.org/?tree=Try&rev=6da0c4532869 (only the test changes, i.e. the patch with the pref back at "show prompt")

Comment 30

[Honza Bambas (:mayhemer)]

Backed out [1] for previously undetected intermittent failure [2].

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/503e2b1df1da
[2] https://tbpl.mozilla.org/php/getParsedLog.php?id=26545877&tree=Mozilla-Inbound

Comment 31

[Honza Bambas (:mayhemer)]

Attachment: v2.3 (only test fixes)

More changes then just a quick fix to land w/o a review.. sorry.

Updates *only in tests*, no C++ or chrome code changes:
- each offline test (dom/ajax/offline) now waits (asynchronously) for any pending update to finish first before the test finishes it self (new teardownAndFinish(callback) method of OfflineTest, offlineTests.js file)
- used in all tests that use OfflineTest.setup()
- only in test_bug460353.html I directly use OfflineTest.waitForUpdates(callback)
- removed some debug dumps() but few useful left

The cause of the mochitest-3 failure was that test_bug460353.html finished sometimes sooner then all updates invoked by that test and its subpages finished (the logic of the test cannot catch them all).  Thus we cleared the cache before a running update filled it again.  That lead to some tests later ended with unexpected "noupdate" event and not with expected "cached" event.

One could say that we should delete cache at the start of each test.  But that is not easily doable.  And we do clean the cache at the end of each test.  The code to wait for updates is anyway a good improvement, it may well fix few other intermittent failures.  BTW, I was suspecting after-test-continuing updates were cause of many troubles.

https://tbpl.mozilla.org/?tree=Try&rev=723f207fc0d9
https://tbpl.mozilla.org/?tree=Try&rev=78ecce166bff (with the pref at "show prompt")

Comment 32

[(no longer active)]

Comment on attachment 790823 [details] [diff] [review]
v2.3 (only test fixes)

Review of attachment 790823 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!  r=me

Comment 33

[Honza Bambas (:mayhemer)]

Comment on attachment 790823 [details] [diff] [review]
v2.3 (only test fixes)

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1ad1b82974

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d1ad1b82974

Comment 35

[Alice0775 White]

I think release note is necessary.

Comment 36

[Lukas Blakk [:lsblakk] use ?needinfo]

Note this in the developer tag - since that's the reasoning for removing it.

Comment 38

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #28)
> (In reply to Honza Bambas (:mayhemer) from comment #21)
> > (In reply to Dão Gottwald [:dao] from comment #20)
> > > (In reply to Honza Bambas (:mayhemer) from comment #3)
> > > > Backed out
> > > > https://hg.mozilla.org/integration/mozilla-inbound/rev/7f177c80f3b9 since we
> > > > have tests that check the prompt :))
> > > 
> > > Can we remove the pref and the prompt after we've shipped this?
> > 
> > Potentially, but since there is some kind of a risk, I'd leave it at least
> > one or two cycles.
> 
> Can you file a bug on this so we don't lose track of it?

Comment 39

[Honza Bambas (:mayhemer)]

Dão, what exactly is the question here?

Comment 40

[Dão Gottwald [:dao]]

This bug left code behind that we have no intention to re-enable, i.e. it's dead code. Right? Has this code been removed by now? If not, has a bug been filed on this? If not, can you do that?

Comment 41

[Honza Bambas (:mayhemer)]

(In reply to Dão Gottwald [:dao] from comment #40)
> This bug left code behind that we have no intention to re-enable, i.e. it's
> dead code. Right? Has this code been removed by now? If not, has a bug been
> filed on this? If not, can you do that?

Aha!  I think some people want this option left, so I would not remove it, at least for now.  I think we will one day remove the whole current appcache support, and will go with it.