[@ NS_GetInnermostURI - nsDocShell::ValidateOrigin]

RESOLVED FIXED

Status

()

Core
Document Navigation
--
blocker
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: timeless, Assigned: dveditz)

Tracking

({crash, regression, topcrash})

Trunk
x86
Windows XP
crash, regression, topcrash
Points:
---
Bug Flags:
blocking1.9 ?
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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 :)
Flags: blocking1.9?
(Reporter)

Comment 1

10 years ago
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.
Keywords: topcrash
(Reporter)

Comment 2

10 years ago
Created attachment 314866 [details] [diff] [review]
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.
Assignee: dveditz → timeless
Status: NEW → ASSIGNED
Attachment #314866 - Flags: review?(dveditz)
(Assignee)

Comment 3

10 years ago
Created attachment 314867 [details] [diff] [review]
Watch out for URI-less system principals
Attachment #314867 - Flags: superreview?(jst)
Attachment #314867 - Flags: review?(jst)
(Assignee)

Comment 4

10 years ago
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.
Attachment #314867 - Flags: approval1.9?
Attachment #314867 - Flags: superreview?(jst)
Attachment #314867 - Flags: superreview+
Attachment #314867 - Flags: review?(jst)
Attachment #314867 - Flags: review+
(Assignee)

Comment 5

10 years ago
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.
(Assignee)

Updated

10 years ago
Assignee: timeless → dveditz
Status: ASSIGNED → NEW
Can we get a test?
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!
Attachment #314867 - Flags: approval1.9? → approval1.9+
(Reporter)

Updated

10 years ago
Attachment #314866 - Attachment description: use null checks → use null checks (identical to the next patch)
Attachment #314866 - Attachment is obsolete: true
Attachment #314866 - Flags: review?(dveditz)
(Assignee)

Comment 8

10 years ago
Created attachment 315301 [details] [diff] [review]
chrome mochitest
(Assignee)

Comment 9

10 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: in-testsuite+
Crash Signature: [@ NS_GetInnermostURI - nsDocShell::ValidateOrigin]
You need to log in before you can comment on or make changes to this bug.