Closed Bug 486398 Opened 11 years ago Closed 11 years ago

Crash [@ nsHTMLIFrameElement::QueryInterface]

Categories

(Core :: XML, defect, critical)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: smaug)

Details

(Keywords: crash, fixed1.9.1, verified1.9.0.11, Whiteboard: [sg:critical?])

Crash Data

Attachments

(5 files)

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
Attached patch guess fixSplinter Review
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.
Uploaded to tryserver, look for go_ifr somewhere in https://build.mozilla.org/tryserver-builds/?C=M;O=D
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.
Attachment #370515 - Flags: superreview?(jst)
Attachment #370515 - Flags: review?(jst)
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..
Attachment #370515 - Flags: superreview?(jst)
Attachment #370515 - Flags: superreview+
Attachment #370515 - Flags: review?(jst)
Attachment #370515 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/c368d263fd1b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.
Flags: blocking1.9.1?
Flags: blocking1.9.0.10?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.10?
Flags: blocking1.9.0.10+
Whiteboard: [sg:critical?]
Flags: blocking1.9.1? → blocking1.9.1+
Attached patch for 1.9.1Splinter Review
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.
Or perhaps I should add nsPIDOMWindow_1_9_0 interface.
Attached patch v1 for 1.9.0Splinter Review
Assignee: nobody → Olli.Pettay
(In reply to comment #11)
> Or perhaps I should add nsPIDOMWindow_1_9_0 interface.
Though, this wouldn't be any less-ugly solution.
Martijn, could you perhaps test the patch for 1.9.0?
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.
Attachment #373314 - Flags: review?(jst)
Attachment #373314 - Flags: superreview+
Attachment #373314 - Flags: review?(jst)
Attachment #373314 - Flags: review+
Whiteboard: [sg:critical?] → [sg:critical?] patch needs 1.9.0 testing by Martijn, 1.9.0 approval request
Attachment #373314 - Flags: approval1.9.0.10?
Comment on attachment 373314 [details] [diff] [review]
v1 for 1.9.0

martijn: please respond to comment 14
Attachment #373314 - Flags: review?(martijn.martijn)
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.
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.
Attachment #374241 - Flags: review?(martijn.martijn)
Attachment #374241 - Flags: approval1.9.0.11?
Attachment #373314 - Flags: review?(martijn.martijn)
Attachment #373314 - Flags: approval1.9.0.11?
Attachment #374241 - Flags: review?(martijn.martijn) → review+
Whiteboard: [sg:critical?] patch needs 1.9.0 testing by Martijn, 1.9.0 approval request → [sg:critical?]
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
Attachment #374241 - Flags: approval1.9.0.11? → approval1.9.0.11+
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
Keywords: fixed1.9.0.11
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).
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next-
Attached patch 1.8 versionSplinter Review
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.
Crash Signature: [@ nsHTMLIFrameElement::QueryInterface]
Group: core-security
You need to log in before you can comment on or make changes to this bug.