318276 - Crash [@ nsXULDocument::LoadOverlay ]

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Jeff Walden [:Waldo]]

See testcase at URL and click the bottom button to get a crash; I'd attach it in the bug except that it would require two attachments instead of one.

Basically, when an overlay load fails, nsXULDocument::LoadOverlay tries to remove the observer it added to the observer hashtable earlier.  The observer was only added if aObserver wasn't nsnull, but the observer is unconditionally removed if the overlay load fails.  Consequently, if you set up a situation where the overlay load will fail (such as when the overlay isn't found or a security exception occurs [the latter demo'd in the testcase]) and pass in a null observer you'll get a crash.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Add a null-check

Comment 2

[Jesus Cea]

I can confirm the crash.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051130 Firefox/1.5 ID:2005113004

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 204531 [details] [diff] [review]
Add a null-check

-    if (NS_FAILED(rv))
-      mOverlayLoadObservers.Remove(uri); // remove the observer if LoadOverlayInternal generated an error
+    if (aObserver && NS_FAILED(rv))
+        mOverlayLoadObservers.Remove(uri); // remove the observer if LoadOverlayInternal generated an error

Seems like the more correct fix here would be to check if mOverlayLoadObservers is initialized (i.e. if (NS_FAILED(rv) && mOverlayLoadObserver.IsInitialized()) before trying to remove from it, not whether the observer is null.

r+sr=jst with that change.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Addresses comment

Attaching updated patch with reviews carried over...

Comment 5

[Jeff Walden [:Waldo]]

Revised patch checked in on trunk by timeless:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsXULDocument.cpp&branch=&root=/cvsroot&subdir=mozilla/content/xul/document/src&command=DIFF_FRAMESET&rev1=1.682&rev2=1.683

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 205660 [details] [diff] [review]
Addresses comment

This patch has been baking on trunk a couple days; to the best of my knowledge it hasn't introduced any new bugs.  (Indeed, given the extremely small amount of code changed and the way in which it was changed I would be highly surprised if one had been introduced.)

This is an obvious and simple patch to fix a crasher, so I think this should be in Firefox 1.5.0.1.  Requesting approval...

Comment 7

[Mike Schroepfer]

Let's bake a couple more days on the trunk and then consider for 1.8.0.1.

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 205660 [details] [diff] [review]
Addresses comment

a=dveditz for drivers.
Please land on both 1.8.0.1 and 1.8.1 branches, and add the appropriate "fixed" keywords when checked-in.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

1_8:
mozilla/content/xul/document/src/nsXULDocument.cpp; new revision: 1.664.2.6;
1_8_0:
mozilla/content/xul/document/src/nsXULDocument.cpp; new revision: 1.664.2.5.2.1;

Comment 10

[Marcia Knous [:marcia]]

verified fixed on the branching using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1. Clicking on the bottom button in the URL cited does not produce a crash.

Comment 11

[Adam Guthrie]

Verified FIXED using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20060111 Firefox/1.5 and the testcase on Jeff's web site. (Also doesn't crash in a branch build from today.)