Closed
Bug 387446
Opened 17 years ago
Closed 17 years ago
[FIXr]crash [@ NS_GetInnermostURI]
Categories
(Core :: Security: CAPS, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: ispiked, Assigned: bzbarsky)
References
Details
(Keywords: crash, topcrash)
Crash Data
Attachments
(1 file)
(deleted),
patch
|
dveditz
:
review+
jst
:
superreview+
jst
:
approval1.9+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•17 years ago
|
||
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?
Assignee | ||
Comment 2•17 years ago
|
||
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...
Assignee | ||
Comment 3•17 years ago
|
||
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 | ||
Updated•17 years ago
|
Assignee: nobody → dveditz
Component: Style System (CSS) → Security: CAPS
QA Contact: style-system → caps
Assignee | ||
Updated•17 years ago
|
Assignee: dveditz → bzbarsky
Summary: crash [@ NS_GetInnermostURI] → [FIX]crash [@ NS_GetInnermostURI]
Comment 5•17 years ago
|
||
(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)
Assignee | ||
Comment 6•17 years ago
|
||
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.
Assignee | ||
Comment 7•17 years ago
|
||
Tomorrow's builds should have the fix for bug 387572 in them.
Comment 8•17 years ago
|
||
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
Assignee | ||
Comment 9•17 years ago
|
||
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?
Comment 10•17 years ago
|
||
(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.
Assignee | ||
Comment 11•17 years ago
|
||
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 12•17 years ago
|
||
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+
Reporter | ||
Comment 13•17 years ago
|
||
This is the #2 topcrash in Socorro for a7. Would be nice to get this reviewed and landed...
Severity: normal → critical
Keywords: topcrash
Assignee | ||
Comment 14•17 years ago
|
||
What's Socorro? Is it doing things a la comment 11?
Maybe we should just make Write() and Read() on nsPrincipal throw for a7.
Comment 15•17 years ago
|
||
(In reply to comment #14)
> What's Socorro?
It's the code name for the crash-stats.mozilla.com webtool.
Assignee | ||
Comment 16•17 years ago
|
||
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.
Updated•17 years ago
|
Flags: blocking1.9?
Updated•17 years ago
|
Attachment #271605 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 17•17 years ago
|
||
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?
Assignee | ||
Updated•17 years ago
|
Summary: [FIX]crash [@ NS_GetInnermostURI] → [FIXr]crash [@ NS_GetInnermostURI]
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Updated•17 years ago
|
Attachment #271605 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 18•17 years ago
|
||
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
Reporter | ||
Comment 19•17 years ago
|
||
Actually, it does look like this patch fixed the crash. See http://crash-stats.mozilla.com/report/list?range_unit=weeks&version=Firefox%3A3.0a8pre&range_value=2&signature=NS_GetInnermostURI(nsIURI*).
->VERIFIED
Status: RESOLVED → VERIFIED
Updated•13 years ago
|
Crash Signature: [@ NS_GetInnermostURI]
You need to log in
before you can comment on or make changes to this bug.
Description
•