Closed Bug 387446 Opened 17 years ago Closed 17 years ago

[FIXr]crash [@ NS_GetInnermostURI]

Categories

(Core :: Security: CAPS, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: ispiked, Assigned: bzbarsky)

References

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(1 file)

This crash started appearing in Breakpad between 2007-07-08 and 2007-07-09. The interesting thing is that it occurs in older builds IDs; e.g. 2007070209. So its most likely cause is an extension's code updating, not a change in Firefox's code. 0 NS_GetInnermostURI(nsIURI *) 1 nsScriptSecurityManager::CheckLoadURIWithPrincipal(nsIPrincipal *,nsIURI *,unsigned int) 2 CSSLoaderImpl::CheckLoadAllowed(nsIURI *,nsIPrincipal *,nsIURI *,nsISupports *) 3 CSSLoaderImpl::LoadStyleLink(nsIContent *,nsIURI *,nsAString_internal const &,nsAString_internal const &,int,nsICSSLoaderObserver *,int *) 4 nsStyleLinkElement::DoUpdateStyleSheet(nsIDocument *,nsICSSLoaderObserver *,int *,int *,int) 5 nsStyleLinkElement::UpdateStyleSheet(nsICSSLoaderObserver *,int *,int *) 6 nsXULDocument::InsertXMLStylesheetPI(nsXULPrototypePI const *,nsINode *,unsigned int,nsIContent *) 7 nsXULDocument::CreateAndInsertPI(nsXULPrototypePI const *,nsINode *,unsigned int) 8 nsXULDocument::PrepareToWalk() 9 nsXULDocument::OnPrototypeLoadDone(int)
Uh... no idea. The simplest way NS_GetInnermostURI would break is if a null URI is passed to it. But nsStyleLinkElement::DoUpdateStyleSheet never passes a null URI to LoadStyleLink, and the URI gets passed straight through. I don't suppose we can get line numbers out of breakpad or something?
OK, looks like the crash is happening on line 1295 of nsNetUtil, which is consistent with a null URI being passed to NS_GetInnermostURI. The call to NS_GetInnermostURI is on line 1272 of nsScriptSecurityManager, which is passing in the _source_ URI. The one that came out of the nsIPrincipal. The one that nsScriptSecurityManager asserts is non-null...
Hopefully, this will make the crashes go away. With any luck, it'll cause this extension to be fixed. Is there a way to find out what extensions might have updated on the relevant day?
Attachment #271605 - Flags: superreview?(jst)
Attachment #271605 - Flags: review?(dveditz)
Assignee: nobody → dveditz
Component: Style System (CSS) → Security: CAPS
QA Contact: style-system → caps
Assignee: dveditz → bzbarsky
Summary: crash [@ NS_GetInnermostURI] → [FIX]crash [@ NS_GetInnermostURI]
(In reply to comment #3) > Is there a way to find out what extensions might have updated on the relevant > day? > Hi bz, i think this caused by myFirefox Theme https://addons.mozilla.org/en-US/firefox/addon/4129 Steps to Reproduce for a reproducible crash. 1. Install myFirefox Theme https://addons.mozilla.org/en-US/firefox/addon/4129 2. Use this theme as default 3. Restart Minefield 4. Install a second theme like Vista Areo https://addons.mozilla.org/en-US/firefox/addon/4988 (its currently not compatible with Trunk 1.9a7) 5. Restart the Trunk build via the Restart Button in Tools -> Addons -> Themes -> Restart Minefield 6. On Restart the Crash Reporter comes up -> crash on restarts and you can only start Firefox now in Safe Mode http://crash-stats.mozilla.com/report/index/8c312863-2efe-11dc-be05-001a4bd43e5c?date=2007-07-10-15 Stack of Crashing Thread frame signature 0 NS_GetInnermostURI(nsIURI *) 1 nsScriptSecurityManager::CheckLoadURIWithPrincipal(nsIPrincipal *,nsIURI *,unsigned int) 2 CSSLoaderImpl::CheckLoadAllowed(nsIURI *,nsIPrincipal *,nsIURI *,nsISupports *) 3 CSSLoaderImpl::LoadStyleLink(nsIContent *,nsIURI *,nsAString_internal const &,nsAString_internal const &,int,nsICSSLoaderObserver *,int *) 4 nsStyleLinkElement::DoUpdateStyleSheet(nsIDocument *,nsICSSLoaderObserver *,int *,int *,int) 5 nsStyleLinkElement::UpdateStyleSheet(nsICSSLoaderObserver *,int *,int *) 6 nsXULDocument::InsertXMLStylesheetPI(nsXULPrototypePI const *,nsINode *,unsigned int,nsIContent *) 7 nsXULDocument::CreateAndInsertPI(nsXULPrototypePI const *,nsINode *,unsigned int) 8 nsXULDocument::PrepareToWalk() 9 nsXULDocument::OnPrototypeLoadDone(int)
Hmm... I actually can't reproduce the bug with those steps. But I had to fix bug 387572 before my build would start after installing this theme. I suppose the two could be related... Once that lands, would you be willing to retest? And I still think the patch in this bug is a good idea -- it improves OOM handling, if nothing else.
Tomorrow's builds should have the fix for bug 387572 in them.
crash with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a7pre) Gecko/200707110505 Minefield/3.0a7pre and my steps to reproduce on new profiles: http://crash-stats.mozilla.com/report/index/290f3d80-2fbb-11dc-ab91-001a4bd43e5c?date=2007-07-11-14
Just to make sure I understand. When I install the "Vista-aero" theme, it actually fails to install because the trunk's version number has changed. After it has failed to install, the restart is supposed to crash?
(In reply to comment #9) > Just to make sure I understand. When I install the "Vista-aero" theme, it > actually fails to install because the trunk's version number has changed. > After it has failed to install, the restart is supposed to crash? > yes, when the install of the Vista-Areo theme fails, (and you use the myFirefox Theme as default theme), just restart Firefox with the Restart Button in the extension manager under tools -> add-ons -> themes and it crash here.
OK, with an optimized build I can reproduce the problem. The basic issue is that someone is loading chrome://global/skin/myFirefoxTab/myTab-bar-right.xul in a docshell. We go to fastload it, try to deserialize its principal (which is an nsPrincipal, because this is a skin package), hit bug 369566, and end up creating a principal with null URIs, etc. So we should really fix that bug. That said, I still like the patch here, so I think we should get that in as well.
Depends on: 369566
Comment on attachment 271605 [details] [diff] [review] Let's try for some sanity checks in this code... r=dveditz
Attachment #271605 - Flags: review?(dveditz) → review+
This is the #2 topcrash in Socorro for a7. Would be nice to get this reviewed and landed...
Severity: normal → critical
Keywords: topcrash
What's Socorro? Is it doing things a la comment 11? Maybe we should just make Write() and Read() on nsPrincipal throw for a7.
(In reply to comment #14) > What's Socorro? It's the code name for the crash-stats.mozilla.com webtool.
Ah, I see. It's not socorro crashing. Gotcha. ;) In order of increasing risk, doing what I suggest in comment 14 is simplest and safest. This patch is next, and the fix for bug 369566 is next.
Flags: blocking1.9?
Attachment #271605 - Flags: superreview?(jst) → superreview+
Comment on attachment 271605 [details] [diff] [review] Let's try for some sanity checks in this code... Add some sanity nullchecks to the script security manager to maybe deal better with people trying to pass around null URIs. Risk is fairly low.
Attachment #271605 - Flags: approval1.9?
Summary: [FIX]crash [@ NS_GetInnermostURI] → [FIXr]crash [@ NS_GetInnermostURI]
Flags: blocking1.9? → blocking1.9+
Attachment #271605 - Flags: approval1.9? → approval1.9+
Fixed. Note that I doubt this fixes the crash; I think bug 369566 is needed for that.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Crash Signature: [@ NS_GetInnermostURI]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: