466314 - updatingImplicit.html (used by test_offlineMode.html and test_updatingManifest.html) crashes SeaMonkey

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect, P2)

Description

[Serge Gautherie (:sgautherie)]

For example:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1227373562.1227376537.14624.gz
Linux comm-central dep unit test on 2008/11/22 09:06:02
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1227368685.1227374271.7217.gz
MacOSX 10.4 comm-central dep unit test on 2008/11/22 07:44:45
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1227368459.1227377887.19869.gz
Win2k3 comm-central dep unit test on 2008/11/22 07:40:59
And I could reproduce it on my Windows 2000.

***

If I remove
|manifest="http://localhost:8888/tests/dom/tests/mochitest/ajax/offline/updating.cacheManifest"|
from updatingImplicit.html
the test (hangs but) doesn't crash anymore.

*****

I will disable the 2 affected tests for now.

Helpwanted to find out what is wrong with the test/file and/or SeaMonkey.

Comment 1

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)
> I will disable the 2 affected tests for now.

http://hg.mozilla.org/mozilla-central/rev/f1af606531f5

Comment 2

[Robert Kaiser]

Do you have crash stacks for this? I guess those could help a lot...

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)

I don't: no installed debugger, and I don't know if breakpad can be activated and would help.

Fwiw,
|python runtests.py --test-path=dom/tests/mochitest/ajax/offline/test_offlineMode.html --autorun|

Comment 4

[Honza Bambas (:mayhemer)]

taking a look at this.

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: v1

nsOfflineCacheUpdate instance is released too soon when it has not been modified. SeaMonkey is probably missing some hook or component that refers it as in Firefox. I changed it to be a ref ptr because I believe it has to be and I free it after an event has been sent to the update (its method has been called). I keep it in a local ref ptr to safely keep reference to it until the method exits. The same function as kungFuDeathGrip.

Comment 6

[Serge Gautherie (:sgautherie)]

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

>-    mUpdate->ManifestCheckCompleted(aStatus, manifestHash);
>+    nsRefPtr<nsOfflineCacheUpdate> update = mUpdate;
>+    mUpdate = nsnull;
>+    if (update)
>+        update->ManifestCheckCompleted(aStatus, manifestHash);

|mUpdate = nsnull;| could be inside the |if|.
Maybe |nsRefPtr<nsOfflineCacheUpdate> update = mUpdate;| too, if that would be safe.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Addressed Serge's comment.

Comment 8

[Honza Bambas (:mayhemer)]

This blocks important tests for offline stuff.

Comment 9

[Dave Camp (:dcamp)]

I'm still building seamonkey to confirm, but I think the problem actually lies in the fact that nsOfflineCacheUpdate::Finish() eventually calls nsOfflineCacheUpdateService::UpdateFinished().  That drops the (potentially) last reference, destroying the nsOfflineCacheUpdate.

In two places (LoadCompleted() when we need to schedule an implicit entry update and ManifestCheckCompleted() when we reschedule the update) we try to continue after Finish().  We should make sure these two spots hold a reference to |this| until they're done executing.

Comment 10

[Dave Camp (:dcamp)]

This should block, because I suspect it's possible to cause this crash in firefox if you don't reference window.applicationCache.

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: v2

Using NS_ProxyRelease from :Finish() method to always have a reference pass this method.

Comment 12

[Dave Camp (:dcamp)]

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

Actually, it's probably better to just ref before the ProxyRelease;  better to leak than crash.

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: v2.1

Yeah, better leak, I have already heard that rule somewhere ;)

Comment 14

[Dave Camp (:dcamp)]

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

Looks good, but could you include a comment explaining why we need this?

Comment 15

[Honza Bambas (:mayhemer)]

Attachment: v2.1, with a comment

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 358085 [details] [diff] [review]
v2.1, with a comment

 nsresult
 nsOfflineCacheUpdate::Finish()
 {
     LOG(("nsOfflineCacheUpdate::Finish [%p]", this));
 
+    // Because call to service->UpdateFinished(this) at the end of this method
+    // may relese the last reference to this object but we still want to work
+    // with it after Finish() call ended, make sure to release this instance in
+    // the next thread loop round.
+    NS_ADDREF_THIS();
+    NS_ProxyRelease(NS_GetCurrentThread(), this, PR_TRUE);
+

So the way we generally deal with this type of problem in Mozilla code is to have the function, or functions, up the stack hold a strong reference on the object while making this call. If you search for kungFuDeathGrip in the codebase you'll find plenty of cases where we do similar things. Would that not be a better option here, than relying on our proxy code to delay this release?

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to comment #16)
We decided to use this approach because:
1. first version of the patch adds a circular reference that potentially might not be broken and we could leak on every cache update attempt
2. kungFuDeathGrip have to be used in all methods that call Finish() method and want to use this after that, so when we add a new method using Finish() and that change got landed without kungFuDeathGrip we may crash again
3. adding proxy release ensures that we never crash and we also put less effort to coders and reviewers to catch point 2 mistake

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 358085 [details] [diff] [review]
v2.1, with a comment

Ok, given those constraints I'd be ok with doing the release this way here. But be aware, if someone ever goes back to the event loop after calling Finish(), but before de-referencing the pointer on which Finish() was called, we'd have the same problem. Much less likely, of course, but also pretty unlikely to be caught by a reviewer.

Comment 19

[Honza Bambas (:mayhemer)]

So far, landed on trunk as http://hg.mozilla.org/mozilla-central/rev/b82c1c2d061e

Comment 20

[Honza Bambas (:mayhemer)]

Backed out from trunk, tests timeout, cannot reproduce locally.

Comment 21

[Honza Bambas (:mayhemer)]

Attachment: v2.1, with a comment and test fix

API changed between last test and land. This is re-tested and test updated.

Comment 22

[Honza Bambas (:mayhemer)]

Attachment: v2.1, with a comment and test fix [Checkin: Comment 23]

Sorry, wrong symlink.

Comment 23

[Honza Bambas (:mayhemer)]

Landed on trunk as http://hg.mozilla.org/mozilla-central/rev/d3af73afeb44

Comment 24

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090129 SeaMonkey/2.0a3pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/9864a75ee19e + bug 474152 patch
 +http://hg.mozilla.org/comm-central/rev/f61570d094c5)

Would still crash.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090129 SeaMonkey/2.0a3pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/01c42e286b4c
 +http://hg.mozilla.org/comm-central/rev/dcb38165108d)

V.Fixed

Comment 25

[Honza Bambas (:mayhemer)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7c453d19f1ae