760067 - Release all OfflineCache custom profile files ASAP after custom profile cache update has finished

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

In bug 753990 comment 32 Felipe raised an issue that index.sqlite file created in the custom profile directory cannot be deleted while Firefox process is still running.  

That also means two processes may keep the file open concurrently when the web app bound to that profile is started sooner Firefox is closed.

Thus, we should shut the custom offline cache device ASAP the cache is activated (or discarded) by nsApplicationCache::Active() (or ::Discard()).

Comment 1

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

Honza, any chance we can get this for FF16? Sorry, I should have poked sooner but was away for vacation.

Comment 2

[Honza Bambas (:mayhemer)]

It's the first bug I'm about to work on now.

Comment 3

[Jason Smith [:jsmith]]

We need this fixed for FF 16.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 5

[Michal Novotny [:michal]]

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

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

::: netwerk/cache/nsDiskCacheDeviceSQL.h
@@ +258,5 @@
>    nsCOMPtr<nsIFile>               mBaseDirectory;
>    nsCOMPtr<nsIFile>               mCacheDirectory;
>    PRUint32                        mCacheCapacity; // in bytes
>    PRInt32                         mDeltaCounter;
> +  bool                            mAutoShutdown;

mAutoShutdown is not initialized in constructor

Comment 6

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

Honza, Felipe, once this bug is fixed, is there anything that we need to do on the desktop-webapp-installation side to take advantage of the fix. I.e. do we need to add any explicit calls to close the appcache or some such? Or will things "magically work" as soon as this bug is fixed?

Comment 7

[Honza Bambas (:mayhemer)]

Things should megically work, since I made the code to close the database when the new cache version is either activated (success) or discarded (failure or no-update).  However, manual testing with all integrated would be good.  I can do it, but I don't know what all patches still needs to land and how to test some actual web app installation.

Comment 8

[Jason Smith [:jsmith]]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> Things should megically work, since I made the code to close the database
> when the new cache version is either activated (success) or discarded
> (failure or no-update).  However, manual testing with all integrated would
> be good.  I can do it, but I don't know what all patches still needs to land
> and how to test some actual web app installation.

I'll be able to take a look at this (I'm the QA on desktop web apps). When this patch lands, what should I look out for in relation to appcache?

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Jason Smith [:jsmith] from comment #8)
> I'll be able to take a look at this (I'm the QA on desktop web apps). When
> this patch lands, what should I look out for in relation to appcache?

First, check a web app loads from app cache (i.e. shut the server down if you can or simply disconnect the machine you test on from internet and load it while Firerox is still open).  Then, if you can, check that after an app has been loaded in the WebRT "foreign" web app's profile Firefox does not any longer keep the file /path/to/just-installed-web-app-profile/OfflineCache/index.sqlite.  That should do.

Comment 10

[Honza Bambas (:mayhemer)]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2838ba825479

Comment 11

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

http://mozillamemes.tumblr.com/post/21637966463/yes-i-have-a-condition-that-forces-me-to-insert

One of the patches was causing xpcshell failures. Since I didn't see Try results in any of the bugs, I had to backout the entire push.
https://hg.mozilla.org/integration/mozilla-inbound/rev/981ac887f6e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/925562fd7d68

An example:
https://tbpl.mozilla.org/php/getParsedLog.php?id=13404043&tree=Mozilla-Inbound

TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_cacheForOfflineUse_no-store.js | running test ...
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_cacheForOfflineUse_no-store.js | test failed (with xpcshell return code: 0), see following log:
>>>>>>>

TEST-INFO | (xpcshell/head.js) | test 1 pending

TEST-INFO | (xpcshell/head.js) | test 2 pending

TEST-INFO | (xpcshell/head.js) | test 2 finished

TEST-INFO | (xpcshell/head.js) | running event loop

TEST-INFO | (xpcshell/head.js) | test 2 pending
TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_cacheForOfflineUse_no-store.js | Starting test_normal

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/head.js | [Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_cacheForOfflineUse_no-store.js :: make_channel_for_offline_use :: line 21"  data: no] - See following stack:
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: do_throw :: line 451
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: _run_next_test :: line 899
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: <TOP_LEVEL> :: line 418

TEST-INFO | (xpcshell/head.js) | exiting test

TEST-INFO | (xpcshell/head.js) | test 2 finished
<<<<<<<

Comment 12

[Honza Bambas (:mayhemer)]

I forgot to update test_cacheForOfflineUse_no-store.js to use the new API.  It is simple to fix, locally it works.

However, now the patch conflicts with Thinker's patch for bug 769291 that has in the mean time landed on m-i.

The conflicting case happens, however, only when the cache update fails.  In that case the related web app cannot be started anyway.

I will split the patch to two parts:
1. shut the device down after successful activation (update done) ; I will land it today
2. shut the device down after discard of the new cache (no update or update/first load failure) ; will be delayed: I have to test that more deeply because of interthread races and also since that part is not important right now since web apps don't have so far (AFAIK) an update mechanism being run from firefox periodically

Comment 13

[Honza Bambas (:mayhemer)]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a055d955691

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3a055d955691

Comment 15

[Jason Smith [:jsmith]]

Verified on Nightly on Win 7. After install of fabrice's test app for appcache (http://people.mozilla.com/~fdesre/openwebapps/test.html), I cut the internet connection. Then, I launched the app. In the app profile, no folder called OfflineCache existed containing a sqlite file.

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Jason Smith [:jsmith] from comment #15)
> Verified on Nightly on Win 7. After install of fabrice's test app for
> appcache (http://people.mozilla.com/~fdesre/openwebapps/test.html), I cut
> the internet connection. Then, I launched the app. In the app profile, no
> folder called OfflineCache existed containing a sqlite file.

Not sure this is the proper way to verify this bug.  

This bug ensures that after we've successfully cached a web app from Firefox, any file in web app's OfflineCache dir is no longer open by Firefox.  But the files in OfflineCache dir under the web app's profile dir must be present.

> In the app profile, no
> folder called OfflineCache existed containing a sqlite file

If you've seen this behavior then something is wrong.

Comment 17

[Jason Smith [:jsmith]]

Oh. Moving back to resolved fixed then. I wonder if I'm looking in the right spot then. I'm looking in:

C:\Users\jsmith\AppData\Roaming\http;people.mozilla.com\Profiles\plcsruws.default

Comment 18

[:Felipe Gomes (needinfo for replies!)]

A simple way to verify this is:

- install webapp that has appcache support
- don't close firefox and don't open the app
- try to delete webapp profile (the folder that you pointed above)

you should be able to successfully delete the profile

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to Felipe Gomes (:felipe) from comment #18)

Thanks, that's it!

> you should be able to successfully delete the profile

On Windows, Linux allows deletion of opened files, AFAIK.  If you are on Linux, you have to check what files Firefox keeps open.

Comment 20

[Jason Smith [:jsmith]]

(In reply to Felipe Gomes (:felipe) from comment #18)
> A simple way to verify this is:
> 
> - install webapp that has appcache support
> - don't close firefox and don't open the app
> - try to delete webapp profile (the folder that you pointed above)
> 
> you should be able to successfully delete the profile

Makes sense. Thanks for the pointers. Verified on Windows. I'll test on Linux to be sure.

Comment 21

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #20)
> (In reply to Felipe Gomes (:felipe) from comment #18)
> > A simple way to verify this is:
> > 
> > - install webapp that has appcache support
> > - don't close firefox and don't open the app
> > - try to delete webapp profile (the folder that you pointed above)
> > 
> > you should be able to successfully delete the profile
> 
> Makes sense. Thanks for the pointers. Verified on Windows. I'll test on
> Linux to be sure.

Looks good on Linux as well. Running ls -l /proc/<PID> for firefox-bin didn't show anything referencing the profile directory for the app.