119310 - preferences + open in new tab + clicking browser crashes @nsLoadGroup::RemoveRequest

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[timeless]

Segfault :) cvs debug build from yesterday no really interesting patches, 

#0  0x28b96890 in nsLoadGroup::RemoveRequest (this=0x8a8c400, request=0x0,
ctxt=0x0, 
    aStatus=0) at /home/timeless/mozilla/netwerk/base/src/nsLoadGroup.cpp:491
#1  0x293161ad in nsXULDocument::ResumeWalk (this=0x8b11c00)
    at /home/timeless/mozilla/content/xul/document/src/nsXULDocument.cpp:5968
#2  0x2930219c in nsXULDocument::EndLoad (this=0x8b11c00)
    at /home/timeless/mozilla/content/xul/document/src/nsXULDocument.cpp:1689
#3  0x292f557f in XULContentSinkImpl::DidBuildModel (this=0x8643000,
aQualityLevel=0)
    at /home/timeless/mozilla/content/xul/document/src/nsXULContentSink.cpp:538
#4  0x288937fb in nsExpatDriver::DidBuildModel (this=0x8373280, anErrorCode=0, 
    aNotifySink=1, aParser=0x8643500, aSink=0x8643000)
    at /home/timeless/mozilla/htmlparser/src/nsExpatDriver.cpp:806
#5  0x288a96fe in nsParser::DidBuildModel (this=0x8643500, anErrorCode=0)
    at /home/timeless/mozilla/htmlparser/src/nsParser.cpp:1385
#6  0x288aa93d in nsParser::ResumeParse (this=0x8643500, allowIteration=1, 
    aIsFinalChunk=1, aCanInterrupt=1)
    at /home/timeless/mozilla/htmlparser/src/nsParser.cpp:1892
#7  0x288ac68d in nsParser::OnStopRequest (this=0x8643500, request=0x84daf80, 
    aContext=0x0, status=0) at
/home/timeless/mozilla/htmlparser/src/nsParser.cpp:2516
#8  0x28c0bbed in nsJARChannel::OnStopRequest (this=0x84daf80, 
    jarExtractionTransport=0x86dce04, context=0x0, aStatus=0)
    at /home/timeless/mozilla/netwerk/protocol/jar/src/nsJARChannel.cpp:611
#9  0x28c2f36d in nsOnStopRequestEvent::HandleEvent (this=0x862ab80)
    at /home/timeless/mozilla/netwerk/base/src/nsRequestObserverProxy.cpp:212
#10 0x28b9c4e0 in nsARequestObserverEvent::HandlePLEvent (plev=0x862ab80)
    at /home/timeless/mozilla/netwerk/base/src/nsRequestObserverProxy.cpp:115
#11 0x282c03e1 in PL_HandleEvent (self=0x862ab80)
    at /home/timeless/mozilla/xpcom/threads/plevent.c:590
#12 0x282c01c8 in PL_ProcessPendingEvents (self=0x80fed80)
    at /home/timeless/mozilla/xpcom/threads/plevent.c:520
#13 0x282c2678 in nsEventQueueImpl::ProcessPendingEvents (this=0x80fed40)
    at /home/timeless/mozilla/xpcom/threads/nsEventQueue.cpp:388
#14 0x289bb7f6 in event_processor_callback (data=0x80fed40, source=6, 
    condition=GDK_INPUT_READ) at
/home/timeless/mozilla/widget/src/gtk/nsAppShell.cpp:184
#15 0x289bb385 in our_gdk_io_invoke (source=0x82bed90, condition=G_IO_IN,
data=0x82bed80)
    at /home/timeless/mozilla/widget/src/gtk/nsAppShell.cpp:77
#16 0x2854165c in g_io_unix_dispatch () from /usr/local/lib/libglib12.so.3
#17 0x28542d03 in g_main_dispatch () from /usr/local/lib/libglib12.so.3
#18 0x2854332c in g_main_iterate () from /usr/local/lib/libglib12.so.3
#19 0x285434c4 in g_main_run () from /usr/local/lib/libglib12.so.3
#20 0x284638b7 in gtk_main () from /usr/X11R6/lib/libgtk12.so.2
#21 0x289bbf89 in nsAppShell::Run (this=0x80cd2f0)
    at /home/timeless/mozilla/widget/src/gtk/nsAppShell.cpp:349
#22 0x2894d2fd in nsAppShellService::Run (this=0x80d1080)
    at /home/timeless/mozilla/xpfe/appshell/src/nsAppShellService.cpp:302
#23 0x0805a4ec in main1 (argc=1, argv=0xbfbff5e8, nativeApp=0x0)
    at /home/timeless/mozilla/xpfe/bootstrap/nsAppRunner.cpp:1264
#24 0x0805b235 in main (argc=1, argv=0xbfbff5e8)
    at /home/timeless/mozilla/xpfe/bootstrap/nsAppRunner.cpp:1594
#25 0x08055655 in _start ()

483     nsLoadGroup::RemoveRequest(nsIRequest *request, nsISupports* ctxt,
nsresult aStatus)
484     {
485         nsresult rv;
486
487     #if defined(PR_LOGGING)
488             PRUint32 count = 0;
489             (void)mRequests->Count(&count);
490             nsXPIDLString nameStr;
491             request->GetName(getter_Copies(nameStr));
492             LOG(("LOADGROUP [%x]: Removing request %x %s status %x
(count=%d).\n",
493                 this, request, NS_ConvertUCS2toUTF8(nameStr).get(), aStatus,
count-1));
494     #endif
495
496         //
497         // Remove the request from the group.  If this fails, it means that
498         // the request was *not* in the group so do not update the
foreground
499         // count or it will get messed up...
500         //
501         //
502         // XXX this method incorrectly returns a bool
503         //
504         rv = mRequests->RemoveElement(request) ? NS_OK : NS_ERROR_FAILURE;
505         if (NS_FAILED(rv)) {
506             LOG(("LOADGROUP [%x]: Unable to remove request %x. Not in
group!\n",
507                 this, request));
508             return rv;
509         }
510
511         nsLoadFlags flags;
512         rv = request->GetLoadFlags(&flags);
513         if (NS_FAILED(rv)) return rv;

(gdb) up
#1  0x293161ad in nsXULDocument::ResumeWalk (this=0x8b11c00)
    at /home/timeless/mozilla/content/xul/document/src/nsXULDocument.cpp:5968
5968            rv = group->RemoveRequest(mPlaceHolderRequest, nsnull, NS_OK);


The steps are something like:
http://bugzilla.mozilla.org
click view bugs reported today
context click the reso dupl ctrl-t bug
select open in new tab
edit>preferences
click browser window

I think right now we won't crash nondebug because i suspect removealement will
return failure, but i'm not certain. it doesn't matter, this is easily fixed.

Comment 1

[timeless]

fwiw the reason for this crash is probably that i wasn't building the new
content pack extension so chrome://content-packs/content/prefOverlay.xul failed
to load.

Comment 2

[Brendan Eich [:brendan]]

Why is it valid for request to be null if an overlay is missing?  Or have I
misunderstood?

/be

Comment 3

[Brendan Eich [:brendan]]

Shaver loves bogo-dependencies on optional-by-definition extensions/*, so I'm
cc'ing him.  I sure hope that extensions/content-packs is not mandatory now, or
we'll have to go break its legs.

/be

Comment 4

[timeless]

proposed fix:
+if (mPlaceHolderRequest) {
5954        // Remove the placeholder channel; if we're the last channel in the
(gdb)
5955        // load group, this will fire the OnEndDocumentLoad() method in the
5956        // docshell, and run the onload handlers, etc.
5957        nsCOMPtr<nsILoadGroup> group = do_QueryReferent(mDocumentLoadGroup);
5958        if (group) {
5959            rv = group->RemoveRequest(mPlaceHolderRequest, nsnull, NS_OK);
5960            if (NS_FAILED(rv)) return rv;
5961
5962            mPlaceHolderRequest = nsnull;
5963        }
+ }
5964        return rv;

while crawling up the stack, i ran into this:
1383        if(mParserContext && !mParserContext->mPrevContext) {
1384          if (mParserContext->mDTD) {
1385            result = mParserContext->mDTD->DidBuildModel(anErrorCode,PRBool
(0==mParserContext->mPrevContext),this,mSink);

Which is silly, so i'd like to make the following change:
!1385           result = mParserContext->mDTD->DidBuildModel
(anErrorCode,PR_TRUE,this,mSink);

I'll also add an ensureargpointer (or an assert) to RemoveRequest.

waterson: the significant code i'm changing is mostly yours.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Could you attach your proposed fix as a patch, or at the very least say what
file the changes go in?

Comment 6

[timeless]

Attachment: proposed fix

Comment 7

[Chris Waterson]

The parser changes are fine. Have you done any analysis to determine why
mPlaceholderRequest is null?

Comment 8

[timeless]

i can explain from an abstract perspective what mozilla was doing, but i got
very lost trying to see how it managed to do it.

overlayinfo was referencing content packs.  content packs were no longer in
chrome/ so upon opening preferences, mozilla tried to load 
chrome://content-packs/content/prefOverlay.xul which failed, and somehow
resulted in what we have here.

Comment 9

[Chris Waterson]

I guess I'm just not convinced that this is the right solution. On the one hand,
it's fun to wallpaper around crashes like this with null checks so we don't
crash, but on the other hand, it might be hiding a deeper problem.

If you want to add an assert-botch when mPlaceholderDocument == nsnull, and then
safely bail out, that's okay. Leave this bug open and reassign it to me in that
case.

As I said above, the parser changes are fine. I don't think the load group
changes are necessary or particularly useful, but I'll let darin decide on that.

Comment 10

[Darin Fisher]

i'm okay with the additional nsLoadGroup runtime check... especially since load
groups are going to be part of the frozen mozilla 1.0 interface.

Comment 11

[timeless]

assertion checked in. investigation => waterson

Comment 12

[Chris Waterson]

Okay, I just tracked this down (I was hitting it for another reason altogether).
What's happening is that we're failing to get a |proto| for the master document
in nsXULDocument::PrepareToWalk (e.g., because the document is malformed). We
bail from PrepareToWalk without ever setting up |mPlaceHolderRequest|.

So...that's a long winded way of saying, ``timeless's fix is just fine.''
Closing the bug.

Comment 13

[timeless]

Attachment: remove the assertion

I hit the assertion again, but since waterson says my fix is correct i'd like
to stop asserting :-)
nsDebug::Assertion(const char * 0x01add318, const char * 0x01add304, const char
* 0x01add2c8, int 5804) line 280 + 13 bytes
nsXULDocument::ResumeWalk() line 5804 + 47 bytes
nsXULDocument::ParserObserver::OnStopRequest(nsXULDocument::ParserObserver *
const 0x0475a9d8, nsIRequest * 0x0475dbc8, nsISupports * 0x00000000, unsigned
int 2147500037) line 7125 + 11 bytes
nsParser::OnStopRequest(nsParser * const 0x0475c35c, nsIRequest * 0x0475dbc8,
nsISupports * 0x00000000, unsigned int 2147500037) line 2452
nsJARChannel::OnStopRequest(nsJARChannel * const 0x0475dbcc, nsIRequest *
0x0475dd0c, nsISupports * 0x00000000, unsigned int 2147500037) line 606 + 49
bytes
nsOnStopRequestEvent::HandleEvent() line 213