using old handler attribute 'type' causes null ptr crash

RESOLVED FIXED in mozilla0.9.9

Status

()

Core
XBL
--
critical
RESOLVED FIXED
17 years ago
14 years ago

People

(Reporter: Pete Collins (MDG), Assigned: Pete Collins (MDG))

Tracking

({crash, helpwanted})

Trunk
mozilla0.9.9
x86
FreeBSD
crash, helpwanted
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

17 years ago
If you use old style type attribute instead of event, we crash.    

   <handlers>
      <handler type="mousedown" />
    </handlers>

BOOM!

--pete

Updated

17 years ago
Keywords: crash

Comment 1

17 years ago
just posting the stack for ease of reference (to get the crash I just jammed 
such a handler into radio.xml and opened the prefs panel).

nsXBLBinding::InstallEventHandlers(nsXBLBinding * const 0x02949038) line 863 + 
11 bytes
nsXBLService::LoadBindings(nsXBLService * const 0x018ad328, nsIContent * 
0x02882608, const nsAString & {...}, int 0x00000000, nsIXBLBinding * * 
0x0012f5a0, int * 0x0012f594) line 712
nsCSSFrameConstructor::ConstructFrameInternal(nsCSSFrameConstructor * const 
0x00000000, nsIPresShell * 0x02898dd8, nsIPresContext * 0x028d6ec0, 
nsFrameConstructorState & {...}, nsIContent * 0x02882608, nsIFrame * 
0x0291d710, nsIAtom * 0x0108a918, int 0x00000008, nsIStyleContext * 0x02935e80, 
nsFrameItems & {...}, int 0x00000000) line 6923
nsCSSFrameConstructor::ConstructFrame(nsCSSFrameConstructor * const 0x00000000, 
nsIPresShell * 0x02898dd8, nsIPresContext * 0x028d6ec0, nsFrameConstructorState 
& {...}, nsIContent * 0x0108a918, nsIFrame * 0x0291d710, nsFrameItems & {...}) 
line 6885 + 35 bytes
nsCSSFrameConstructor::ContentInserted(nsCSSFrameConstructor * const 
0x010845c8, nsIPresContext * 0x028d6ec0, nsIContent * 0x02882588, nsIContent * 
0x02882608, int 0x00000000, nsILayoutHistoryState * 0x00000000, int 0x00000000) 
line 8707
StyleSetImpl::ContentInserted(StyleSetImpl * const 0x028b62c8, nsIPresContext * 
0x028d6ec0, nsIContent * 0x02882588, nsIContent * 0x02882608, int 0x00000001) 
line 1446
PresShell::ContentInserted(PresShell * const 0x02898de0, nsIDocument * 
0x0287cdc0, nsIContent * 0x02882588, nsIContent * 0x02882608, int 0x00000001) 
line 5157
nsXBLResourceLoader::NotifyBoundElements(nsXBLResourceLoader * const 
0x00000000) line 277
nsXBLResourceLoader::StyleSheetLoaded(nsXBLResourceLoader * const 0x0294fd88, 
nsICSSStyleSheet * 0x0294fd88, int 0x00000001) line 207
CSSLoaderImpl::InsertSheetInDoc(CSSLoaderImpl * const 0x00000000, 
nsICSSStyleSheet * 0x0293f898, int 0x00000002, nsIContent * 0x00000000, int 
0x00000001, nsICSSLoaderObserver * 0x0291ab18) line 1200 + 10 bytes
InsertPendingSheet(void * 0x02909c48, void * 0x0288e1a0) line 763
nsStringArray::EnumerateForwards(nsStringArray * const 0x00000000, int 
(nsString &, void *)* 0x0174b320 InsertPendingSheet(void *, void *), void * 
0x0288e1a0) line 905 + 11 bytes
CSSLoaderImpl::Cleanup(CSSLoaderImpl * const 0x00000000, URLKey & {...}, 
SheetLoadData * 0x0294fdd8) line 827
CSSLoaderImpl::SheetComplete(CSSLoaderImpl * const 0x00000000, nsICSSStyleSheet 
* 0x00000000, SheetLoadData * 0x0294fdd8) line 920
CSSLoaderImpl::ParseSheet(CSSLoaderImpl * const 0x00000000, 
nsIUnicharInputStream * 0x0293e3e8, SheetLoadData * 0x00000000, int & 
0x00000001, nsICSSStyleSheet * & 0x017fb0dc const  CSSParserImpl::`vftable') 
line 955
CSSLoaderImpl::DidLoadStyle(CSSLoaderImpl * const 0x00000000, nsIStreamLoader * 
0x028d42c8, nsString * 0x00000001, SheetLoadData * 0x0293f898, unsigned int 
0x0293e3e8) line 991
SheetLoadData::OnStreamComplete(SheetLoadData * const 0x00000000, 
nsIStreamLoader * 0x028d42c8, nsISupports * 0x00000000, unsigned int 
0x00000000, unsigned int 0x00000b39, const char * 0x0293e4c8) line 751
nsStreamLoader::OnStopRequest(nsStreamLoader * const 0x00000000, nsIRequest * 
0x0294fdd8, nsISupports * 0x00000000, unsigned int 0x00000000) line 163
nsJARChannel::OnStopRequest(nsJARChannel * const 0x02909dbc, nsIRequest * 
0x028abc0c, nsISupports * 0x00000000, unsigned int 0x00000000) line 611 + 30 
bytes
nsOnStopRequestEvent::HandleEvent(nsOnStopRequestEvent * const 0x00000000) line 
213
PL_HandleEvent(PLEvent * 0x0290a05c) line 591
PL_ProcessPendingEvents(PLEventQueue * 0x100340d8) line 520 + 6 bytes
_md_EventReceiverProc(HWND__ * 0x00000001, unsigned int 0x00401fc2, unsigned 
int 0x0100ed50, long 0x00000000) line 1071 + 10 bytes
nsAppShellService::Run(nsAppShellService * const 0x0100ed50) line 303
main1(int 0x00000001, char * * 0x00323c30, nsISupports * 0x00322c30) line 1285 
+ 9 bytes
main(int 0x00000001, char * * 0x00323c30) line 1625 + 26 bytes
WinMain(HINSTANCE__ * 0x00400000, HINSTANCE__ * 0x00400000, char * 0x0013348e, 
HINSTANCE__ * 0x00400000) line 1643 + 21 bytes
MOZILLA! WinMainCRTStartup + 308 bytes
KERNEL32! 77e87903
(Assignee)

Comment 2

17 years ago
Created attachment 65930 [details] [diff] [review]
Fix
(Assignee)

Comment 3

17 years ago
This is an easy fix.

Hyatt can you review?

Thanks

--pete
Assignee: hyatt → petejc
(Assignee)

Comment 4

17 years ago
Created attachment 65931 [details] [diff] [review]
nit: i like this better
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
NS_ERROR_NULL_POINTER supposedly means someone passed a null pointer into a
method that requires a non-null pointer.  You probably want NS_ERROR_UNEXPECTED
or some such.  Is this the only spackle needed to handle old attributes?

/be
(Assignee)

Comment 6

17 years ago
Created attachment 65936 [details] [diff] [review]
returning NS_ERROR_UNEXPECTED;
Attachment #65930 - Attachment is obsolete: true
Attachment #65931 - Attachment is obsolete: true
(Assignee)

Comment 7

17 years ago
> Is this the only spackle needed to handle old attributes?

For events, yes.
I have no idea about other old or depreciated attributes in other xbl contexts.

--pete

Comment 8

17 years ago
By bailing early from that while loop (and method) any valid handlers that 
follow a '<handler type=...' will not be installed. (Note the chaining at 
the end of the while loop).
(Assignee)

Comment 9

17 years ago
Created attachment 65941 [details] [diff] [review]
continue on to next handler
Attachment #65936 - Attachment is obsolete: true
(Assignee)

Comment 10

17 years ago
Yea, thanks for pointing that out john.

If this patch is "good for review", then there are a few minor compiler errors
i'd liek to fix as well in this file. BUt i will submit that after this is
deemed "good".

--pete
Would it not make more sense to re-write that while() loop into a for() loop to
eliminate the duplication of the iteration code? I.e. something like:

    for (; curr; curr->GetNextHandler(getter_AddRefs(next)),
           curr = next) {
      ...
    }

Other than that I'm fine with the change, but let's see a new patch that does
that, unless someone disagrees with me.
jst's got it: continue with for consolidates the update part.

Style nit: I'd lay out his example differently, either indenting the second line
of the update part to underhang starting in the same column as the update part,
or (better) giving each part its own line.  Then the for loop can have a
non-empty initialization part too, couldn't it?

    for (nsCOMPtr<nsIXBLPrototypeHandler> curr = handlerChain;
         curr;
         curr->GetNextHandler(getter_AddRefs(next)), curr = next) {
      ...
    }

giving a canonical for loop.

/be
Yeah, even better. Declaring |curr| in the for loop does let people with broken
compilers break the tree by using |curr| outside the loop, but we'll notice that
soon enough...
(Assignee)

Comment 14

17 years ago
Created attachment 65984 [details] [diff] [review]
changing to for loop and removing redundency
Attachment #65941 - Attachment is obsolete: true
(Assignee)

Comment 15

17 years ago
Brendan, cur and next are declared before the for loop.
Just in case this will break on certain compilers as jst pointed out.

--pete
What Brendan suggested won't break on any compiler, but it does let people make
mistakes in the future on broken compilers by using |cur| outside the loop. I'm
ok either way.
pete: nothing breaks in my proposed change to put the declaration in the for
loop's initialization part.  The hazard that leaves, if people are careless, is
that MSVC will let you use the variable after the loop body, while the C++
standard and conforming compilers will not.  I think in this function it's ok to
take the chance, but others should decide.

Anyway, there's nothing wrong with declarating a variable in the for-loop init
part, if you use a standard-conforming compiler, or watch tinderbox.

/be
(Assignee)

Comment 18

17 years ago
Created attachment 66039 [details] [diff] [review]
declaration and initializer inside the for loop

kNameSpaceSeparator is not used and was causing compiler warning.
using int instead of raw number -1 to ease compiler complaint.

Compiles cleanly now. 

--pete
(Assignee)

Comment 19

17 years ago
BTW: what is the advantage of keeping crr and next inside the scope of the for loop?
Less instruction movement on the stack? Better micro perf?

 --pete
The advantage is that the code does what you intend it to do, it doesn't leak
variables outside the scope where the variables are intended to be used (except
in broken compilers). Not sure there are any real benefits at the code
generation level.
Comment on attachment 66039 [details] [diff] [review]
declaration and initializer inside the for loop

Shouldn't the compiler warning be fixed with a cast in stead of a temporary
variable? Either way, sr=jst
Attachment #66039 - Flags: superreview+
Comment on attachment 66039 [details] [diff] [review]
declaration and initializer inside the for loop

I think you should get rid of that negOne int temporary -- just pass
PRUint32(-1).  But this begs the question of why PRUint32 was used for the
aIndex formal parameter to nsXBLInsertionPoint and NS_NewXBLInsertionPoint,
when mIndex is PRInt32.  hyatt, can you field that one and r= here?

BTW, the MSVC bug where you can use i after for (int i = 0; i < n; i++) {...}
is really a hazard, in my experience, for integer variables with canonical
names like i -- people who develop on Windows tend to reuse such variables
without checking where the declaration is.  But for a variable such as next or
curr in this context, there isn't as much of a hazard.	But hyatt gets to
decide the likelihood there too.

/be
(Assignee)

Comment 23

17 years ago
Created attachment 66052 [details] [diff] [review]
casting PRUint32 for -1 numerical constant
Attachment #65984 - Attachment is obsolete: true
Attachment #66039 - Attachment is obsolete: true

Updated

17 years ago
Attachment #66052 - Attachment description: casting PRUint31 for -1 numerical constant → casting PRUint32 for -1 numerical constant
(Assignee)

Updated

17 years ago
Target Milestone: --- → mozilla0.9.9
(Assignee)

Comment 24

17 years ago
Brendan can you r= this so i can check it in?

I'll die of old age waiting for Hyatt to look at it.  ;-)

Thanks

--pete
Comment on attachment 66052 [details] [diff] [review]
casting PRUint32 for -1 numerical constant

transferring jst's sr= from the last patch, adding r=brendan@mozilla.org.

/be
Attachment #66052 - Flags: superreview+
Attachment #66052 - Flags: review+
(Assignee)

Comment 26

17 years ago
Thanks Brendan, i'll get it in there when the tree reopens.

--pete
(Assignee)

Comment 27

17 years ago
Chekced in.

Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.