434035 - Crash [@ nsGlobalWindow::FindInternal] when doing find on a deleted window

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes trunk and branch builds after 300ms.

http://crash-stats.mozilla.com/report/index/c897db66-232e-11dd-8adc-001cc45a2ce4?p=1
0  	xul.dll  	nsGlobalWindow::FindInternal  	 mozilla/dom/src/base/nsGlobalWindow.cpp:6305
1 	xul.dll 	nsGlobalWindow::Find 	mozilla/dom/src/base/nsGlobalWindow.cpp:6285
2 	xul.dll 	NS_InvokeByIndex_P 	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
3 	xul.dll 	XPCWrappedNative::CallMethod 	mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2388

Comment 1

[Martijn Wargers (dead)]

Attachment: patch

This patch adds a null check for this bug and bug 434037.

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 321270 [details] [diff] [review]
patch

- In nsGlobalWindow::ShowModalDialog():

+  if (!mDocShell)
+    return NS_ERROR_FAILURE;

You could do this earlier than this, i.e. right after setting *aRetVal to null, and use NS_ENSURE_TRUE here as you do in the find code.

r+sr=jst

Comment 3

[Martijn Wargers (dead)]

Attachment: patchv2

With review comments addressed.

Comment 4

[Martijn Wargers (dead)]

Comment on attachment 321335 [details] [diff] [review]
patchv2

I have no idea what to do with the patch. Tinderbox says I need to land it in trunk/Hg (?).

Comment 5

[Doug Turner (:dougt)]

this should have landed a while ago.  It would have also fixed 434037.  martijn, can you land this if it get approved?

Comment 6

[Johnny Stenback (:jst)]

This is not a 1.9.1 blocker, but it's a trivial null check so there's no reason not to take it. I approved the patch, please land asap. Also opening this bug up to the public as this is simply a null dereference crash, so not an exploitable crash.

Comment 8

[Johnny Stenback (:jst)]

The patch in dupe (434037) has another null check in it that we might as well incorporate here when landing this.

Comment 9

[Reed Loden [:reed]]

Attachment: What I am landing [Checkin: Comment 10 & 11]

Unbitrotted patch that includes the other null-check from bug 434037.

Comment 10

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/032320654a33

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 352983 [details] [diff] [review]
What I am landing
[Checkin: Comment 10 & 11]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b1c1991e4af0

Comment 12

[Marcia Knous [:marcia]]

verified fixed on the 1.9.1 branch using  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081222 Shiretoko/3.1b3pre. Adding verified keyword.

Comment 13

[Marcia Knous [:marcia]]

Verified fixed on the trunk using  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081222 Minefield/3.2a1pre. No crash using testcase.

Comment 15

[Samuel Sidler (old account; do not CC)]

Per bug 492980, this happens on Firefox 3.0.x as well. Is this something we should fix in a 1.9.0.x release?

Comment 16

[Johnny Stenback (:jst)]

It's a perfectly safe patch, so I wouldn't be worried about taking it, but it's also a non-exploitable crash, and a pretty rare one at that. So it's not strictly necessary for 1.9.0, but again, I'd be fine with taking it...

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 352983 [details] [diff] [review]
What I am landing
[Checkin: Comment 10 & 11]

Approved for 1.9.0.12, a=dveditz for release-drivers

Comment 18

[Daniel Veditz [:dveditz]]

Checking in dom/src/base/nsGlobalWindow.cpp;
/cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v  <--  nsGlobalWindow.cpp
new revision: 1.1020; previous revision: 1.1019

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.12 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.12pre) Gecko/2009070105 GranParadiso/3.0.12pre (.NET CLR 3.5.30729). Testcase no longer crashes as it does in 1.9.0.11.

Comment 20

[Martin Stránský [:stransky] (ni? me)]

Reproducible even on 1.8.0.

Comment 21

[Jan Horak [:jhorak]]

Attachment: Patch for 1.8.0 version

Adding patch for 1.8.0 version. Thanks for your review in advance.

Comment 22

[Martijn Wargers (dead)]

Comment on attachment 387409 [details] [diff] [review]
Patch for 1.8.0 version

I haven't test the patch in a 1.8.1 build, but it seems fine to me.
Don't you also need to use this part?
+  NS_ENSURE_TRUE(mDocShell, NS_ERROR_FAILURE);
..which the original patch has?

Comment 23

[Jan Horak [:jhorak]]

(In reply to comment #22)
> (From update of attachment 387409 [details] [diff] [review])
> I haven't test the patch in a 1.8.1 build, but it seems fine to me.
> Don't you also need to use this part?
> +  NS_ENSURE_TRUE(mDocShell, NS_ERROR_FAILURE);
> ..which the original patch has?

Thanks. There's no nsGlobalWindow::ShowModalDialog method in 1.8.0 version so I skipped this code. I tried the test case and with the patch it's working well.

Comment 24

[Martijn Wargers (dead)]

Ah, yeah, of course, that part of the patch was for bug 434037, which uses a feature that isn't on 1.8.1 branch, afaik.