Closed Bug 466314 Opened 13 years ago Closed 13 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: sgautherie, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

(Keywords: crash, fixed1.9.1)

Attachments

(1 file, 6 obsolete files)

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.
Flags: wanted1.9.1?
(In reply to comment #0)
> I will disable the 2 affected tests for now.

http://hg.mozilla.org/mozilla-central/rev/f1af606531f5
Do you have crash stacks for this? I guess those could help a lot...
(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|
taking a look at this.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #349663 - Flags: review?(dcamp)
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.
Attached patch v1.1 (obsolete) — Splinter Review
Addressed Serge's comment.
Attachment #349663 - Attachment is obsolete: true
Attachment #350787 - Flags: review?(dcamp)
Attachment #349663 - Flags: review?(dcamp)
This blocks important tests for offline stuff.
Whiteboard: [has patch][needs review 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.
This should block, because I suspect it's possible to cause this crash in firefox if you don't reference window.applicationCache.
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1
Attached patch v2 (obsolete) — Splinter Review
Using NS_ProxyRelease from :Finish() method to always have a reference pass this method.
Attachment #350787 - Attachment is obsolete: true
Attachment #358062 - Flags: review?(dcamp)
Attachment #350787 - Flags: review?(dcamp)
Attachment #358062 - Flags: superreview?(jst)
Attachment #358062 - Flags: review?(dcamp)
Attachment #358062 - Flags: review+
Comment on attachment 358062 [details] [diff] [review]
v2

Actually, it's probably better to just ref before the ProxyRelease;  better to leak than crash.
Attachment #358062 - Flags: superreview?(jst)
Attachment #358062 - Flags: review-
Attachment #358062 - Flags: review+
Attached patch v2.1 (obsolete) — Splinter Review
Yeah, better leak, I have already heard that rule somewhere ;)
Attachment #358062 - Attachment is obsolete: true
Attachment #358081 - Flags: review?(dcamp)
Attachment #358081 - Flags: superreview?(jst)
Attachment #358081 - Flags: review?(dcamp)
Attachment #358081 - Flags: review+
Comment on attachment 358081 [details] [diff] [review]
v2.1

Looks good, but could you include a comment explaining why we need this?
Attached patch v2.1, with a comment (obsolete) — Splinter Review
Attachment #358081 - Attachment is obsolete: true
Attachment #358085 - Flags: superreview?(jst)
Attachment #358081 - Flags: superreview?(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?
(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 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.
Attachment #358085 - Flags: superreview?(jst) → superreview+
Attachment #358085 - Flags: approval1.9.1?
Backed out from trunk, tests timeout, cannot reproduce locally.
API changed between last test and land. This is re-tested and test updated.
Attachment #358085 - Attachment is obsolete: true
Attachment #358085 - Flags: approval1.9.1?
Sorry, wrong symlink.
Attachment #359343 - Attachment is obsolete: true
Landed on trunk as http://hg.mozilla.org/mozilla-central/rev/d3af73afeb44
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review dcamp]
Attachment #359346 - Attachment description: v2.1, with a comment and test fix → v2.1, with a comment and test fix [Checkin: Comment 23]
[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
Status: RESOLVED → VERIFIED
Flags: wanted1.9.1? → in-testsuite+
Target Milestone: mozilla1.9.1 → mozilla1.9.2a1
Duplicate of this bug: 466597
Blocks: 514067
You need to log in before you can comment on or make changes to this bug.