428288 - [@ NS_GetInnermostURI - nsDocShell::ValidateOrigin]

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[timeless]

180 NS_IMETHODIMP 
181 nsSystemPrincipal::GetURI(nsIURI** aURI)
182 {
183     *aURI = nsnull;
184     return NS_OK;
185 }

Assuming GetURI will result in a non null uri is clearly not a good idea.

Signature	NS_GetInnermostURI(nsIURI*)
UUID	6fa70ccc-06e9-11dd-8a6c-001cc4e2bf68
Time	2008-04-10 02:30:32-07:00
Uptime	3424
Product	Firefox
Version	3.0pre
Build ID	2008040907
OS	Windows NT
OS Version	5.1.2600 Service Pack 2
CPU	x86
CPU Info	GenuineIntel family 6 model 13 stepping 8
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x0
Comments	
Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
ntdll!KiUserExceptionDispatcher+0xe
xul!NS_GetInnermostURI(class nsIURI * uri = 0x00000000)+0x1b [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\obj-fx-trunk\dist\include\necko\nsnetutil.h @ 1410]
xul!nsDocShell::ValidateOrigin(class nsIDocShellTreeItem * aOriginTreeItem = 0x0384ffd4, class nsIDocShellTreeItem * aTargetTreeItem = 0x00f972b4)+0x1b2 [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\docshell\base\nsdocshell.cpp @ 1090]
xul!nsDocShell::CanAccessItem(class nsIDocShellTreeItem * aTargetItem = 0x00f972b4, class nsIDocShellTreeItem * aAccessingItem = 0x0384ffd4, int aConsiderOpener = 1)+0x68 [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\docshell\base\nsdocshell.cpp @ 1972]
xul!nsDocShell::FindItemWithName(wchar_t * aName = 0x0012d204 "_content", class nsISupports * aRequestor = 0x00000000, class nsIDocShellTreeItem * aOriginalRequestor = 0x0384ffd4, class nsIDocShellTreeItem ** _retval = 0x0012d084)+0x160 [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\docshell\base\nsdocshell.cpp @ 2105]
xul!nsDocShell::InternalLoad+0x296a24
xul!nsCOMPtr_base::assign_from_qi(class nsQueryInterface qi = class nsQueryInterface, struct nsID * iid = 0x00000000)+0x40 [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\obj-fx-trunk\xpcom\build\nscomptr.cpp @ 98]
xul!NS_TableDrivenQI(void * aThis = 0x24448b10, struct QITableEntry * entries = 0x0012cef4, struct nsID * aIID = 0x00000000, void ** aInstancePtr = 0x0012cf1c)+0x55 [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\obj-fx-trunk\xpcom\build\nsisupportsimpl.cpp @ 50]
xul!nsHTMLAnchorElement::QueryInterface(struct nsID * aIID = 0xffffb2e8, void ** aInstancePtr = 0x04c25eff)+0x46 [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\content\html\content\src\nshtmlanchorelement.cpp @ 163]

I was using venkman at the time :)

Comment 1

[timeless]

dveditz wanted steps:
1. open venkman
2. click either of:
<http://www.hacksrus.com/~ginda/venkman/faq/venkman-faq.html>
<http://www.mozilla.org/projects/venkman/>

expected results: no crash.
actual results: crash.

Comment 2

[timeless]

Attachment: use null checks (identical to the next patch)

the expected results is that the chrome frame w/ the help text is not a valid match for the target (return false), so the engine will search up until it finds no matches in the window, and then ask the engine if any other windows can handle browsing (true for firefox/seamonkey, false for most others). if it can't find a browsing handler, it'll use exthandler and eventually ask the os to handle the url.

In the old code, since x-jsd!=http, the code returned false. The current code would return false if any of the pointers were null, so let's let them be null.

Comment 3

[Daniel Veditz [:dveditz]]

Attachment: Watch out for URI-less system principals

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 314867 [details] [diff] [review]
Watch out for URI-less system principals

Requesting approval before reviews to see if this is something that will be allowed in.

Comment 5

[Daniel Veditz [:dveditz]]

I don't know how "top" this crash is--it's around #78--but it's a safe fix and a guaranteed crash for any extension that does this kind of thing. #78 crash is a pretty impressive showing if the problem is limited to Venkman help.

Comment 6

[Damon Sicore (:damons)]

Can we get a test?

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 314867 [details] [diff] [review]
Watch out for URI-less system principals

I think Damon meant: tests would be nice, but it's pretty easy to see the value of these null checks!

Comment 8

[Daniel Veditz [:dveditz]]

Attachment: chrome mochitest

Comment 9

[Daniel Veditz [:dveditz]]

Checked in.