Closed Bug 486470 Opened 15 years ago Closed 15 years ago

sporadic crash on Windows tinderboxen in gfxTextRun::~gfxTextRun()

Categories

(Core :: Graphics, defect, P2)

Product:
Core
Component:
Graphics
Platform:
x86
Windows XP
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: jtd, Assigned: dcamp)

References

Details

(Keywords: crash, fixed1.9.1, intermittent-failure)

Attachments

(4 files, 1 obsolete file)

patch, debug trashing of mGlyphRuns array
12.20 KB, patch
Details | Diff | Splinter Review
patch, debug with lots and lots of logging
64.51 KB, patch
Details | Diff | Splinter Review
sample log file
739.99 KB, text/plain
Details
fix
3.60 KB, patch
mayhemer
: review+
vlad
: superreview+
vlad
: approval1.9.1+
Details | Diff | Splinter Review 
Windows unit test machine is periodically crashing in gfxTextRun::~gfxTextRun() when running mochitest-plain tests.

Example logs:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1238606166.1238611997.12746.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1238599133.1238605868.953.gz&fulltext=1

Crash reason:  EXCEPTION_ACCESS_VIOLATION
Crash address: 0x0

Stack crawl:

 0  xul.dll!nsTArray<gfxTextRun::GlyphRun>::~nsTArray<gfxTextRun::GlyphRun>() [nsTArray.h:7f359d93170a : 267 + 0x5]
    eip = 0x101c5059   esp = 0x0012e11c   ebp = 0x0012e174   ebx = 0x0719c401
    esi = 0x0719c428   edi = 0x0ac5841c   eax = 0x00000000   ecx = 0x0719c428
    edx = 0xf57b0012   efl = 0x00010202
 1  xul.dll!gfxTextRun::~gfxTextRun() [gfxFont.cpp:7f359d93170a : 1409 + 0xf]
    eip = 0x10632267   esp = 0x0012e124   ebp = 0x0012e174
 2  xul.dll!gfxTextRun::`vector deleting destructor'(unsigned int) + 0x35
    eip = 0x101bdd3c   esp = 0x0012e12c   ebp = 0x0012e174
 3  xul.dll!nsTextFrame::ClearTextRun() [nsTextFrameThebes.cpp:7f359d93170a : 3692 + 0x7]
    eip = 0x1027213a   esp = 0x0012e138   ebp = 0x0012e174   ebx = 0x0719c418
 4  xul.dll!nsTextFrame::Destroy() [nsTextFrameThebes.cpp:7f359d93170a : 3345 + 0x4]
    eip = 0x102729eb   esp = 0x0012e144   ebp = 0x0012e174   ebx = 0x00000000
 5  xul.dll!nsLineBox::DeleteLineList(nsPresContext *,nsLineList &) [nsLineBox.cpp:7f359d93170a : 337 + 0x7]
    eip = 0x1033d6a8   esp = 0x0012e14c   ebp = 0x0012e174
 6  xul.dll!nsBlockFrame::Destroy() [nsBlockFrame.cpp:7f359d93170a : 298 + 0x9]
    eip = 0x103376f7   esp = 0x0012e158   ebp = 0x0012e174

Going back to 3/20, I don't see the exact same crash but there are other crashes in GlyphRun code:

Example:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1237610333.1237613764.10070.gz&fulltext=1

 0  xul.dll!gfxTextRun::FindFirstGlyphRunContaining(unsigned int) [gfxFont.cpp:49d7fee2e9b4 : 2175 + 0x3]
    eip = 0x1062eefa   esp = 0x0012bc90   ebp = 0x0012bcd4   ebx = 0x00000001
    esi = 0x00000000   edi = 0x00000000   eax = 0x00000000   ecx = 0x0313a1f8
    edx = 0x00000000   efl = 0x00010297
 1  xul.dll!gfxTextRun::GlyphRunIterator::GlyphRunIterator(gfxTextRun *,unsigned int,unsigned int) [gfxFont.h:49d7fee2e9b4 : 1313 + 0xd]
    eip = 0x101bda30   esp = 0x0012bc9c   ebp = 0x0012bcd4
 2  xul.dll!gfxTextRun::MeasureText(unsigned int,unsigned int,gfxFont::BoundingBoxType,gfxContext *,gfxTextRun::PropertyProvider *) [gfxFont.cpp:49d7fee2e9b4 : 1925 + 0xe]
    eip = 0x1062faff   esp = 0x0012bca8   ebp = 0x0012bcd4
 3  xul.dll!gfxTextRun::BreakAndMeasureText(unsigned int,unsigned int,int,double,gfxTextRun::PropertyProvider *,int,double *,gfxFont::RunMetrics *,gfxFont::BoundingBoxType,gfxContext *,int *,unsigned int *,int,gfxBreakPriority *) [gfxFont.cpp:49d7fee2e9b4 : 2103 + 0x21]
    eip = 0x1062fefd   esp = 0x0012bcdc   ebp = 0x0012c418
 4  xul.dll!nsTextFrame::Reflow(nsPresContext *,nsHTMLReflowMetrics &,nsHTMLReflowState const &,unsigned int &) [nsTextFrameThebes.cpp:49d7fee2e9b4 : 5976 + 0x71]
    eip = 0x10275bfe   esp = 0x0012c45c   ebp = 0x0012c5e8   ebx = 0x0b694df0
 5  xul.dll!nsLineLayout::ReflowFrame(nsIFrame *,unsigned int &,nsHTMLReflowMetrics *,int &) [nsLineLayout.cpp:49d7fee2e9b4 : 852 + 0x2e]
    eip = 0x103a037e   esp = 0x0012c654   ebp = 0x0012c72c   ebx = 0x0012c840
 6  xul.dll!nsBlockFrame::ReflowInlineFrame(nsBlockReflowState &,nsLineLayout &,nsLineList_iterator,nsIFrame *,LineReflowStatus *) [nsBlockFrame.cpp:49d7fee2e9b4 : 3594 + 0x13]
    eip = 0x10339af0   esp = 0x0012c79c   ebp = 0x0012c7cc   ebx = 0x0b694f1c
 7  xul.dll!nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState &,nsLineLayout &,nsLineList_iterator,int *,LineReflowStatus *,int) [nsBlockFrame.cpp:49d7fee2e9b4 : 3415 + 0x15]
    eip = 0x1033a364   esp = 0x0012c7d4   ebp = 0x0012c814   ebx = 0x0b694f1c

All these crashes occur running this test:

*** 31828 INFO Running /tests/dom/tests/mochitest/ajax/offline/test_xhtmlManifest.xhtml...

Not sure if there are other factors that match (e.g. same tinderbox machine).
Flags: blocking1.9.1?
Browsing through the Tinderbox logs around 3/14, I don't see this crash, so most likely a change in the past couple weeks.
Happened again:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1238684373.1238693310.29046.gz

(also running tests/dom/tests/mochitest/ajax/offline/test_xhtmlManifest.xhtml)
(In reply to comment #2)
> Possibly related to bug 484177?

I'm guessing these are related.  I went back and did an analysis of Tinderbox crashes, looking for crashes involving glyph runs and test_xhtmlManifest.xhtml.

The crashes in FindFirstGlyphRunContaining started 3/18 and stopped 3/23, then the ~gfxTextRun crashes started 4/1 just after 8am.  Hard to see changesets that relate to this, the only one that seems related to the end of the FindFirstGlyphRunContaining crashes is:

Joe Drew — Bug 484076 - Shrink the glyph cache to fix tp_Rss regression. r=me
http://hg.mozilla.org/mozilla-central/rev/2617fe64e693
Mar 23 18:54:20

I don't see any obvious code change that would induce this bug.

Full history:

WINNT 5.2 mozilla-central unit test on 2009/03/18 21:16:38 
http://hg.mozilla.org/mozilla-central/index.cgi/rev/4e1a4c23041c
gfxTextRun::FindFirstGlyphRunContaining(unsigned int) [gfxFont.cpp:4e1a4c23041c : 2175 + 0x3]

WINNT 5.2 mozilla-central unit test on 2009/03/19 01:42:40 
http://hg.mozilla.org/mozilla-central/index.cgi/rev/032ffaec3006
gfxTextRun::FindFirstGlyphRunContaining(unsigned int) [gfxFont.cpp:032ffaec3006 : 2172 + 0x3]

WINNT 5.2 mozilla-central unit test on 2009/03/19 07:48:08 
http://hg.mozilla.org/mozilla-central/index.cgi/rev/80da01543b56
gfxTextRun::FindFirstGlyphRunContaining(unsigned int) [gfxFont.cpp:80da01543b56 : 2172 + 0x3]

WINNT 5.2 mozilla-central unit test on 2009/03/19 21:29:08 
http://hg.mozilla.org/mozilla-central/index.cgi/rev/ac8e14d8a263
gfxTextRun::FindFirstGlyphRunContaining(unsigned int) [gfxFont.cpp:ac8e14d8a263 : 2172 + 0x3]

WINNT 5.2 mozilla-central unit test on 2009/03/20 18:04:18 
http://hg.mozilla.org/mozilla-central/index.cgi/rev/49d7fee2e9b4
gfxTextRun::FindFirstGlyphRunContaining(unsigned int) [gfxFont.cpp:49d7fee2e9b4 : 2175 + 0x3]

WINNT 5.2 mozilla-central unit test on 2009/03/20 21:38:53 
http://hg.mozilla.org/mozilla-central/index.cgi/rev/49d7fee2e9b4
gfxTextRun::FindFirstGlyphRunContaining(unsigned int) [gfxFont.cpp:49d7fee2e9b4 : 2175 + 0x3]

WINNT 5.2 mozilla-central unit test on 2009/03/21 17:57:24 
http://hg.mozilla.org/mozilla-central/index.cgi/rev/8d2c566f3256
gfxTextRun::FindFirstGlyphRunContaining(unsigned int) [gfxFont.cpp:8d2c566f3256 : 2175 + 0x3]

WINNT 5.2 mozilla-central unit test on 2009/03/23 18:24:55 
http://hg.mozilla.org/mozilla-central/index.cgi/rev/72969ace6b09
gfxTextRun::FindFirstGlyphRunContaining(unsigned int) [gfxFont.cpp:72969ace6b09 : 2172 + 0x3]

WINNT 5.2 mozilla-central unit test on 2009/04/01 08:18:53 
http://hg.mozilla.org/mozilla-central/index.cgi/rev/7f359d93170a
nsTArray<gfxTextRun::GlyphRun>::~nsTArray<gfxTextRun::GlyphRun>() [nsTArray.h:7f359d93170a : 267 + 0x5]

WINNT 5.2 mozilla-central unit test on 2009/04/01 10:16:06 
http://hg.mozilla.org/mozilla-central/index.cgi/rev/479d0bfb14cb
nsTArray<gfxTextRun::GlyphRun>::~nsTArray<gfxTextRun::GlyphRun>() [nsTArray.h:479d0bfb14cb : 267 + 0x5]

WINNT 5.2 mozilla-central unit test on 2009/04/01 20:39:17 
http://hg.mozilla.org/mozilla-central/index.cgi/rev/8d60bedd277b
nsTArray<gfxTextRun::GlyphRun>::~nsTArray<gfxTextRun::GlyphRun>() [nsTArray.h:8d60bedd277b : 267 + 0x5]

WINNT 5.2 mozilla-central unit test on 2009/04/02 01:01:05 
http://hg.mozilla.org/mozilla-central/index.cgi/rev/cc4fae01938c
nsTArray<gfxTextRun::GlyphRun>::~nsTArray<gfxTextRun::GlyphRun>() [nsTArray.h:cc4fae01938c : 267 + 0x5]

WINNT 5.2 mozilla-central unit test on 2009/04/02 01:12:07 
http://hg.mozilla.org/mozilla-central/index.cgi/rev/d3240cd1c610
nsTArray<gfxTextRun::GlyphRun>::~nsTArray<gfxTextRun::GlyphRun>() [nsTArray.h:d3240cd1c610 : 267 + 0x5]

WINNT 5.2 mozilla-central unit test on 2009/04/02 07:59:33 
http://hg.mozilla.org/mozilla-central/index.cgi/rev/ccbfc42dbaaf
nsTArray<gfxTextRun::GlyphRun>::~nsTArray<gfxTextRun::GlyphRun>() [nsTArray.h:ccbfc42dbaaf : 267 + 0x5]
I ran mochitests in a home-made Windows opt build several times and failed to reproduce the crash. I did crash several times in xpcshell (httpd.js server I presume)...
I was going to try running mochitests in a nightly build but then I realized I can't do that.
It'd be very surprising if just changing the Cairo glyph cache size caused crashes. Jeff?
(In reply to comment #6)
> I was going to try running mochitests in a nightly build but then I realized I
> can't do that.

:-( Sadly still correct. 

We're getting closer though, as we continue the "separate-unittest-from-build"
work. See bug#383136 and bug#486783 for details.
(In reply to comment #7)
> It'd be very surprising if just changing the Cairo glyph cache size caused
> crashes. Jeff?

The cairo glyph cache change just seems to have been checked in around the time the first flavor of glyph run associated crash *stopped* occurring.  Somehow that may have perturbed the conditions under which that crash occurred, it's not necessarily the cause of either of the crashes.
I'm able to reproduce this on my WinXP box.

Steps:

1. Apply attached patch to set the textrun cache expiration timeout to 10ms
2. Build with tinderbox-like mozconfig:

export MOZ_DEBUG_SYMBOLS=1
. $topsrcdir/browser/config/mozconfig
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-tinder
ac_add_options --disable-debug
ac_add_options --enable-tests
ac_add_options --enable-optimize
ac_add_options --enable-logrefcnt
mk_add_options MOZ_CO_PROJECT=browser

3. Run dom mochitests:

  TEST_PATH=dom/tests/mochitest make mochitest-plain

Result: 2 out of 3 times this will crash in the same place the crash occurs on the tinderbox.

My guess is that there's a missing refcount increment of a text run somewhere.  Tickling the textrun expiration timeout simulates the behavior running on a mud-slow tinderbox VM.

This is a bit of a heisenbug, if you poke at it the problem disappears.  I tried attaching a debugger while the mochitests were running and that caused the bug not to occur.
That's a great idea ... I'll give it a go
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
I tried that and couldn't reproduce over here :-(.
This patch sets the expire timeout of both the TextRunExpiringCache and the FrameTextRunCache caches to 1ms.  With this setting, the crashes happen frequently with a fully-optimized build, never with a debug build.  Simply running this with a debugger attached causes everything to run fine.  So this patch uses logs to track what's going on:

  NSPR_LOG_FILE=textrun.out
  NSPR_LOG_MODULES=textrun:5,textframe:5

The direct cause of the crash is something is trashing the mHdr field of the mGlyphRuns array, this results in a crash when calling mGlyphRuns->Length().

0[636a48]: textrun: destr (3fc9910) mGlyphRuns dump: 03fc9924 00000001 80000001 01e8eb08 00000000 00008aa1
0[636a48]: textrun: destr (6e78d60) mGlyphRuns dump: 06e78d74 00000001 80000001 02861890 00000000 00008a9d
0[636a48]: textrun: destr (6b8b258) mGlyphRuns dump: 06b8b26c 00000001 80000001 01e56d10 00000000 00008ab0
0[636a48]: textrun: destr (46055a8) mGlyphRuns dump: 046055bc 00000001 80000001 01e56d10 00000000 00008aae
0[636a48]: textrun: destr (403fdc8) mGlyphRuns dump: 0403fddc 00000001 80000001 01e56d10 00000000 00008aac
0[636a48]: textrun: destr (2a55758) mGlyphRuns dump: 02a5576c 00000001 80000001 01e56d10 00000000 00008aaa
0[636a48]: textrun: destr (745abf8) mGlyphRuns dump: 0745ac0c 00000001 80000001 01e56d10 00000000 00008aa9
0[636a48]: textrun: destr (5c8f5a8) mGlyphRuns dump: 05c8f5bc 00000001 80000001 01e56d10 00000000 00008aa8
0[636a48]: textrun: destr (6c8cd30) mGlyphRuns dump: 06c8cd44 00000001 80000001 01e56d10 00000000 00008aa7
0[636a48]: textrun: destr (1bb0638) mGlyphRuns dump: 00000000 00000001 80000001 01e56d10 00000000 00008aa6

The next step is to track down the code trashing this memory.
Attachment #371208 - Attachment is obsolete: true
I didn't manage to repro this yet myself, but I wonder if it is caused by bug 487063. There's a patch there you might like to try.
Try moving mGlyphRuns to the end of gfxTextRun and hex-dump the entire text run. If the mHdr field and *only* the mHdr field is still corrupted the same way, that strongly suggests the corruption is happening via a negative index through mGlyphRuns, although I can't see by looking at the code how that could happen.
I can reproduce this fairly easily now, with the expiration timers set to 1ms (as per comment 14), the crash occurs running a single testcase, the one just before test_xhtmlManifest.xhtml in the dom/tests/mochitest/ajax/offline set of tests.  But the bug is *very* sensitive to memory placement, adding a single long word into gfxTextRun before or after mGlyphRuns prevents the crash.  Likewise, attaching a debugger while running the test also prevents the crash.

Since the crash always occurs when the first longword of mGlyphRuns is set to zero (where the array base is stored), I set up checks to figure out where this longword is getting corrupted.  I bracketed all calls to gfxTextRun methods with corruption checks, and did the same for many methods in nsTextFrame, but the corruption doesn't seem to occur in any of these places, the first place it shows up is when the NotifyExpiry method fires in the FrameTextRunCache, just before the text run is deleted.

There does seem to be a consistent pattern, the same text run gets corrupted each time the crash occurs, a text run with a single space.  Tomorrow I'm going to try to get the exact pattern down and try attaching a debugger when the text run is constructed to see if a data breakpoint can catch the culprit.
(In reply to comment #17)
> There does seem to be a consistent pattern, the same text run gets corrupted
> each time the crash occurs, a text run with a single space.

Hm.. dumb question, I seem to recall that we cache a single space textrun instead of creating it each time, is it that cached one that gets corrupted?
You probably don't want to hear about more of these but in case they are of some use, a couple more from today on WINNT 5.2 mozilla-central unit test - 

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1239219938.1239226093.11649.gz

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1239198392.1239205132.2447.gz
We don't really have a cached single-space textrun.

It would be interesting to know what's in the textrun that gets corrupted, though.
So I tracked this down to some offline cache code.  Based on logging, I set up a debug break as close as possible to when the corruption occurs, then set up data breakpoints.  For some reason, the first longword in the mGlyphRuns struct is getting overwritten within a call to nsOfflineCacheUpdate::Finish().  At the end of that code is:

    nsresult rv = NS_OK;

    if (mOwner) {
        rv = mOwner->UpdateFinished(this);
        mOwner = nsnull;
    }

    return rv;

The optimized assembly for this is:

    if (mOwner) {
104529B6  mov         ecx,dword ptr [esi+10h] 
104529B9  xor         eax,eax 
104529BB  test        ecx,ecx 
104529BD  je          nsOfflineCacheUpdate::Finish+144h (104529C8h) 
        rv = mOwner->UpdateFinished(this);
104529BF  mov         eax,dword ptr [ecx] 
104529C1  push        esi  
104529C2  call        dword ptr [eax] 
        mOwner = nsnull;
104529C4  and         dword ptr [esi+10h],0 
    }

After calling UpdateFinished, ESI is set to the address of a gfxTextRun, so the last instruction above smashes the longword in the mGlyphRuns struct.  Guessing this somehow relates to calls to deallocated objects.
(In reply to comment #21)
>     if (mOwner) {
>         rv = mOwner->UpdateFinished(this);
>         mOwner = nsnull;
>     }

Looking at nsOfflineCacheUpdateService::UpdateFinished (one of the possible things that UpdateFinished could be, I think), it seems like the UpdateFinished call may well cause the last release of its |aUpdate| parameter, which is |this| in the above code, which could make the |mOwner = nsnull| an access to freed memory.

Maybe whoever's calling nsOfflineCacheUpdate::Finish ought to be holding a reference?
So the sequence of events that leads to the memory corruption involves nsOfflineCacheUpdateItem's.  These contain a non-ref-counted pointer to a nsOfflineCacheUpdate object.  One of these is getting destructed but it's still referenced in a nsOfflineCacheUpdateItem.  When nsOfflineCacheUpdateItem::Run is called from an event, it calls the LoadCompleted method of deleted nsOfflineCacheUpdate object and bad things happen.  The exact call stack is:

 	xul.dll!nsOfflineCacheUpdate::Finish()  行 1991	C++
 	xul.dll!nsOfflineCacheUpdate::ProcessNextURI()  行 1601 + 0xb バイト	C++
 	xul.dll!nsOfflineCacheUpdate::LoadCompleted()  行 1453	C++
>	xul.dll!nsOfflineCacheUpdateItem::Run()  行 477	C++
 	xul.dll!nsThread::ProcessNextEvent(int mayWait=0x00000001, int * result=0x0012fc70)  行 511	C++
 	xul.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00000001, int mayWait=0x00000001)  行 230 + 0xd バイト	C++

(行) == line, (バイト) == bytes

I built with the mozconfig described in comment 10, running with the attached debug patch.  The crash occurs with a single testcase:

TEST_PATH=dom/tests/mochitest/ajax/offline/test_updatingManifest.html make mochitest-plain

The debug patch adds logging:

NSPR_LOG_FILE=textrun.out
NSPR_LOG_MODULES=textrun:5,textframe:5,nsOfflineCacheUpdate:5

There are a couple DebugBreak's, the first just before the corruption is about to occur and the second when the corruption has just occurred.
Attached file sample log file — 
For this run, the corruption occur in the text run at 0x03f369c8


0[636a60]: nsOfflineCacheUpdate::NotifyStarted [3f369c8, 2d93058]
0[636a60]: nsOfflineCacheUpdate::LoadCompleted [3f369c8]
0[636a60]: nsOfflineCacheUpdate::NotifyCompleted [3f369c8, 2d93058]
0[636a60]: nsOfflineCacheUpdate::ProcessNextURI [3f369c8, current=1, numItems=1]
0[636a60]: nsOfflineCacheUpdate::Finish begin [3f369c8 thread: 000007c4]
0[636a60]: nsOfflineCacheUpdate::NotifyNoUpdate [3f1a680]
0[636a60]: nsOfflineCacheUpdate destr [3f369c8 thread:000007c4]
0[636a60]: textrun: const (3f369c8) count: 249, ptr: 0x03f369d8 thread: 000007c4 *
0[636a60]: textrun: const (3f369c8) - [1]   (20)
0[636a60]: textrun (3f369c8) AddGlyphRun 1e4d938 0 numRuns: 0
0[636a60]: textrun (3f369c8) AddGlyphRun append 1e4d938 0
0[636a60]: nsOfflineCacheUpdate::Finish begin [3f1a680 thread: 000007c4]
0[636a60]: nsOfflineCacheUpdateService::UpdateFinished [294cf70, update=3f1a680]
0[636a60]: nsOfflineCacheUpdate::RemoveObserver [3f1a680]
0[636a60]: nsOfflineCacheUpdate::RemoveObserver [3f1a680]
0[636a60]: nsOfflineCacheUpdateService::ProcessNextUpdate [294cf70, num=0]
0[636a60]: nsOfflineCacheUpdate::Finish end [3f1a680 thread:000007c4]
0[636a60]: nsOfflineCacheUpdate::Finish end [3f369c8 thread:000007c4]
0[636a60]: textrun (3f369c8) mGlyphRuns corrupt - bad textrun check

So the textrun is using memory previously used for a nsOfflineCacheUpdate object.  The "Finish end [3f369c8" shows part of the call stack executed *after* the destrcution of the nsOfflineCacheUpdate object.

Guessing this was caused by one of the recent updates to the offsite cache code:

http://hg.mozilla.org/mozilla-central/log/907dbdbd084f/uriloader/prefetch/nsOfflineCacheUpdate.cpp
Assignee: roc → nobody
Assignee: nobody → dcamp
The call stack at the time of this stuff would probably be useful in terms of figuring out who should be holding a reference (but isn't).
The log is enough - Finish() takes a ref and then schedules an unref on the main loop.  The idea was to avoid having kungFuDeathGrips for every potential caller of Finish().

But one of the UpdateFinished() implementations eventually fires an event to content, and one of the test handlers spins and event loop with a sync xhr.

So we ought to just use kungFuDeathGrips, I'll put together a patch today.
Attached patch fix — — Splinter Review 

        Attachment #372148 -
        Flags: superreview?(jst)

        Attachment #372148 -
        Flags: review?(honzab.moz)

        Attachment #372148 -
        Attachment is patch: true

        Attachment #372148 -
        Attachment mime type: application/octet-stream → text/plain

    
    

    
  
Comment on attachment 372148 [details] [diff] [review]
fix

I would say that only place we need the grip is nsOfflineCacheUpdate::ManifestCheckCompleted, however better be safer.

Then, why are you using nsCOMPtr<nsI...> instead of nsRefPtr<ns...> ? It would save QI call with the same effect.

        Attachment #372148 -
        Flags: review?(honzab.moz) → review+

    
    

    
  
(In reply to comment #29)
> Then, why are you using nsCOMPtr<nsI...> instead of nsRefPtr<ns...> ? It would
> save QI call with the same effect.

There's no QI call in the use in attachment 372148 [details] [diff] [review].

    
    

    
  
Ah, you are right. Then I have no objections to the patch.

    
    

    
  
Running with the attached patch appears to fix the problem.

This bug is still causing periodic crashes on Windows tinderbox machines, it would be great it we could get this in soon!
Severity: major → critical
Keywords: crash

        Attachment #372148 -
        Flags: superreview?(jst)

        Attachment #372148 -
        Flags: superreview+

        Attachment #372148 -
        Flags: approval1.9.1+

    
    

    
  
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/f0264ef0c795

Pushed to mozilla 1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/43d7823fd64d
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1

    
    

    
  
Happened several times yesterday, no crashes after checkin at 23:09

WINNT 5.2 mozilla-central unit test on 2009/04/13 08:04:38
WINNT 5.2 mozilla-central unit test on 2009/04/13 08:26:08
WINNT 5.2 mozilla-central unit test on 2009/04/13 10:23:53
WINNT 5.2 mozilla-central unit test on 2009/04/13 14:38:35
WINNT 5.2 mozilla-central unit test on 2009/04/13 19:58:41
WINNT 5.2 mozilla-central unit test on 2009/04/13 20:22:19

    
    

    
  
Sounds good to me!
Status: RESOLVED → VERIFIED
Whiteboard: [orange]

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: