Last Comment Bug 760067 - Release all OfflineCache custom profile files ASAP after custom profile cache update has finished
: Release all OfflineCache custom profile files ASAP after custom profile cache...
Status: VERIFIED FIXED
[blocking-webrtdesktop1+], [Desktop W...
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla16
Assigned To: Honza Bambas (:mayhemer)
: Jason Smith [:jsmith]
Mentors:
Depends on: 753990
Blocks: 756620 772919
  Show dependency treegraph
 
Reported: 2012-05-31 05:06 PDT by Honza Bambas (:mayhemer)
Modified: 2012-07-18 17:22 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
?
-


Attachments
v1 (7.51 KB, patch)
2012-07-03 14:15 PDT, Honza Bambas (:mayhemer)
michal.novotny: review+
honzab.moz: checkin+
Details | Diff | Splinter Review

Description Honza Bambas (:mayhemer) 2012-05-31 05:06:10 PDT
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) PTO Until July 5th 2012-07-03 09:53:31 PDT
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) 2012-07-03 09:55:23 PDT
It's the first bug I'm about to work on now.
Comment 3 Jason Smith [:jsmith] 2012-07-03 10:39:03 PDT
We need this fixed for FF 16.
Comment 4 Honza Bambas (:mayhemer) 2012-07-03 14:15:41 PDT
Created attachment 638862 [details] [diff] [review]
v1
Comment 5 Michal Novotny (:michal) 2012-07-09 17:01:02 PDT
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) PTO Until July 5th 2012-07-10 09:12:54 PDT
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) 2012-07-10 09:21:16 PDT
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] 2012-07-10 09:28:10 PDT
(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) 2012-07-10 09:42:21 PDT
(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 11 Ryan VanderMeulen [:RyanVM] 2012-07-10 16:45:27 PDT
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) 2012-07-11 07:26:38 PDT
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 14 Ed Morley [:emorley] 2012-07-12 09:39:03 PDT
https://hg.mozilla.org/mozilla-central/rev/3a055d955691
Comment 15 Jason Smith [:jsmith] 2012-07-16 13:18:24 PDT
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) 2012-07-16 14:08:50 PDT
(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] 2012-07-16 14:15:37 PDT
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 me!) 2012-07-16 14:19:45 PDT
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) 2012-07-16 14:23:14 PDT
(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] 2012-07-16 14:50:33 PDT
(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] 2012-07-16 14:57:10 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.