Closed Bug 428288 Opened 12 years ago Closed 12 years ago

[@ NS_GetInnermostURI - nsDocShell::ValidateOrigin]

Categories

(Core :: DOM: Navigation, defect, blocker)

x86
Windows XP
defect
Not set
blocker

Tracking

()

RESOLVED FIXED

People

(Reporter: timeless, Assigned: dveditz)

References

Details

(Keywords: crash, regression, topcrash)

Crash Data

Attachments

(2 files, 1 obsolete file)

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?
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
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)
Attachment #314867 - Flags: superreview?(jst)
Attachment #314867 - Flags: review?(jst)
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+
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: 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+
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)
Attached patch chrome mochitestSplinter Review
Checked in.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.