486398 - Crash [@ nsHTMLIFrameElement::QueryInterface]

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Martijn Wargers (dead)]

Attachment: zipped up unminimized testcase

I have an unminimized testcase that crashes with this stacktrace. Unfortunately, it does crash not regurlarly, it can take up to 5 minutes to crash, which makes it really difficult to make a minimized testcase, so I'm giving up for now.
I've attached an zipped up unminimized testcase. Open the 'parentframe.htm' file to test. You might be more lucky to get the crash to open that file in multiple tabs.

It also crashes Firefox 3, so marking security sensitive for now.

http://crash-stats.mozilla.com/report/index/7e8f4374-c8c1-4184-9cdf-618842090401?p=1
0  	ntdll.dll  	KiFastSystemCallRet  	
1 	ntdll.dll 	NtReleaseSemaphore 	
2 	kernel32.dll 	ReleaseSemaphore 	
3 	xul.dll 	google_breakpad::ExceptionHandler::WriteMinidumpOnHandlerThread 	toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.cc:562
4 	xul.dll 	xul.dll@0x9254f7 	
5 	mozcrt19.dll 	_purecall 	obj-firefox/memory/jemalloc/src/purevirt.c:47
6 	xul.dll 	nsGenericElement::QueryInterface 	content/base/src/nsGenericElement.cpp:4122
7 	xul.dll 	xul.dll@0x9254f7 	
8 	xul.dll 	nsHTMLIFrameElement::QueryInterface 	content/html/content/src/nsHTMLIFrameElement.cpp:120
9 	xul.dll 	nsCOMPtr_base::assign_from_qi 	obj-firefox/xpcom/build/nsCOMPtr.cpp:96
10 	xul.dll 	nsCOMPtr<nsIContent>::nsCOMPtr<nsIContent> 	obj-firefox/dist/include/xpcom/nsCOMPtr.h:572
11 	xul.dll 	nsGlobalWindow::PostHandleEvent 	dom/base/nsGlobalWindow.cpp:2350
12 	xul.dll 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:294
13 	xul.dll 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:345
14 	xul.dll 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:508
15 	xul.dll 	DocumentViewerImpl::LoadComplete 	layout/base/nsDocumentViewer.cpp:1001
16 	xul.dll 	nsDocShell::EndPageLoad 	docshell/base/nsDocShell.cpp:5263
17 	xul.dll 	nsWebShell::EndPageLoad 	docshell/base/nsWebShell.cpp:1302
18 	xul.dll 	nsCOMPtr_base::assign_from_qi 	obj-firefox/xpcom/build/nsCOMPtr.cpp:98
19 	xul.dll 	nsDocShell::OnStateChange 	docshell/base/nsDocShell.cpp:5159

Firefox 3.0.7 breakpad id:
http://crash-stats.mozilla.com/report/index/1af246df-488c-4ea9-a806-8ad022090401?p=1

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: guess fix

I can't reproduce this on Linux, so the patch is based on the stack trace.
If the patch fixes this, this is probably a regression from
split-window.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Uploaded to tryserver, look for go_ifr somewhere in https://build.mozilla.org/tryserver-builds/?C=M;O=D

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

https://build.mozilla.org/tryserver-builds/2009-04-01_14:[e-mail]-go_ifr/

Comment 4

[Martijn Wargers (dead)]

I've run the testcase now half an hour with the tryserver build and I didn't get the crash, so I'm pretty sure the patch fixes the crash.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 370515 [details] [diff] [review]
guess fix

I did run chrome and browser-chrome tests already yesterday, and there were no leaks.
Running now mochitest..

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/c368d263fd1b

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Hmm, I wonder if I should have updated IID after all.
The member variable is used in some inline methods.

For 1.9.0.x we may want to explicitly addref/release.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for 1.9.1

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/37d1bb0c86b5

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

To fix this on stable branch (1.9.0), I can't change the member from 
nsIDOMElement* to nsCOMPtr<nsIDOMElement>, I think. There are few inline methods 
which use the variable and I'm not sure how the automatic nsCOMPtr release would 
work in case of nsPIDOMWindow destruction.
So, I think the caller of SetFrameElementInternal need to be responsible to
addref/release. That way we don't have to change the interface at all.
Patch coming once I've run all the tests.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Or perhaps I should add nsPIDOMWindow_1_9_0 interface.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v1 for 1.9.0

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #11)
> Or perhaps I should add nsPIDOMWindow_1_9_0 interface.
Though, this wouldn't be any less-ugly solution.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Martijn, could you perhaps test the patch for 1.9.0?

Comment 15

[Boris Zbarsky [:bzbarsky]]

For what it's worth, nsCOMPtr<nsIFoo> and nsIFoo* look the same on the machine level, I think, so the inline methods might have been ok.  And nsPIDOMWindow's destructor is virtual, so doesn't get inlined.  But I agree the other is safer.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 373314 [details] [diff] [review]
v1 for 1.9.0

martijn: please respond to comment 14

Comment 17

[Martijn Wargers (dead)]

I can't reproduce in my 1.9.0.x debug build, so I have to rebuild and then test in a non-debug build. I'll do that tomorrow.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +compilation fix for windows (added a .get())

Comment 19

[Martijn Wargers (dead)]

Olli was so nice to post a tryserver build for this. I can verify that the 1.9.0.x patch seems to fix the crash for this. I let the testcase run in that build in 3 different tabs for one hour. It would have normally crashed for me.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 374241 [details] [diff] [review]
+compilation fix for windows (added a .get())

Approved for 1.9.0.11, a=dveditz for release-drivers

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Checking in content/base/src/nsFrameLoader.cpp;
/cvsroot/mozilla/content/base/src/nsFrameLoader.cpp,v  <--  nsFrameLoader.cpp
new revision: 1.83; previous revision: 1.82
done
Checking in dom/public/base/nsPIDOMWindow.h;
/cvsroot/mozilla/dom/public/base/nsPIDOMWindow.h,v  <--  nsPIDOMWindow.h
new revision: 1.77; previous revision: 1.76
done
Checking in dom/src/base/nsGlobalWindow.cpp;
/cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v  <--  nsGlobalWindow.cpp
new r

Comment 22

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

Verified for 1.9.0.11 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.11pre) Gecko/2009051505 GranParadiso/3.0.11pre (.NET CLR 3.5.30729).

Comment 24

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

Attachment: 1.8 version

Backported 1.8 version + modification for old gcc:

+  nsIDOMElement* fe = frame_element.get(); 
+  NS_ADDREF(fe); 

It's because the current patch doesn't compile on our ancient systems like RHEL-3/2.1.