362716 - [FIX]crash in [@ nsXBLPrototypeHandler::AppendHandlerText]

Status: VERIFIED FIXED

(Core :: XBL, defect, P1)

Description

[arno renevier]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.7) Gecko/20060830 Firefox/1.5.0.7 (Debian-1.5.dfsg+1.5.0.7-2)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.7) Gecko/20060830 Firefox/1.5.0.7 (Debian-1.5.dfsg+1.5.0.7-2)

Hi, I noticed a crash. It happens when in a xbl file you have a handler element with a command attribute if you are not in chrome.
 See url test case for an example.

I investigated and discovered that that crash happens in nsXBLPrototypeHandler::AppendHandlerText (file content/xbl/src/nsXBLPrototypeHandler.cpp)

in nsXBLContentSink::OnOpenContainer (file content/xbl/src/nsXBLContentSink.cpp) 
there is : 

  if (command && !mIsChromeOrResource)
    // Make sure the XBL doc is chrome or resource if we have a command
    // shorthand syntax.
    return; // Don't even make this handler.

so in case command attribute is set, and url scheme is not chrome (or ressource), mHandler is not created.

but in nsXBLContentSink::FlushText (called from nsXBLContentSink::HandleEndElement), there is :
    if (mSecondaryState == eXBL_InHandler) {
      mHandler->AppendHandlerText(text);

AppendHandler method of mHandler is called but mHandler has not been created (and is therefor still nsnuss).
So, my browser crashes.
I make a small patch that seems to solve the problem, but may we could issue a console warning or do something more appropriate.

Reproducible: Always

Steps to Reproduce:
1. go to http://ffsearchplugins.free.fr/bugzilla/example.xul
Actual Results:  
crash

Expected Results:  
should not crash :)

Comment 1

[arno renevier]

Attachment: mentionned patch

Comment 2

[[e-mail]]

As a wild guess I think it might be better if mSecondaryState didn't lie.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yeah, definitely.  The problem is that the current code (with or without the proposed null-check) will stick the text in the wrong handler if there's a handler preceding the "command" handler in question...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: XBL for testcase

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Treat this situation as an error and bail on the binding.  Also treat OOM constructing a handler as an error.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I should note that there's no need to cc me on XBL bugs if you properly reassign them -- I watch the default XBL assignee.  But you do need to properly reassign...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fix checked in.  Not sure what to do with branches; localization is frozen, right?  I suppose I could just not report the error there (and silently bail).

Comment 9

[Jay Patel [:jay]]

bz:  Let's fix the crash and not worry about reporting the error for the branches.

Comment 10

[Daniel Veditz [:dveditz]]

Still need a branch patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch patch

Comment 12

[Jay Patel [:jay]]

Comment on attachment 250795 [details] [diff] [review]
Branch patch

Approved for the branches, a=jay for drivers.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Fixed on both branches.

Comment 14

[Marcia Knous [:marcia]]

verified fixed on the 1.8.0 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.10pre) Gecko/20070111 Firefox/1.5.0.10pre. No crash using http://ffsearchplugins.free.fr/bugzilla/example.xul.

Also verified fixed on the 1.8 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.2pre) Gecko/20070109 BonEcho/2.0.0.2pre. No crash using http://ffsearchplugins.free.fr/bugzilla/example.xul.

Adding the relevant verified keywords.

Comment 15

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070207 Minefield/3.0a3pre