If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Crash [@ nsPermissionManager::GetHost]

RESOLVED FIXED in mozilla1.8beta1

Status

()

Core
Networking: Cookies
--
critical
RESOLVED FIXED
14 years ago
6 years ago

People

(Reporter: timeless, Assigned: db48x)

Tracking

({assertion, crash, topcrash})

Trunk
mozilla1.8beta1
assertion, crash, topcrash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(4 obsolete attachments)

(Reporter)

Description

14 years ago
I'm trying to modify browser.xml, and managed to mess up. I shouldn't crash.

[4540] 0[2a4910]: file chrome://global/content/bindings/browser.xml, line 398: 
TypeError: this.webNavigation has no properties
[4540] 0[2a4910]: file (null), line 0: uncaught exception: 
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) 
[nsIControllers.removeController]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  
location: "JS frame :: chrome://navigator/content/navigator.js :: Shutdown :: 
line 736"  data: no]

 	cookie.dll!nsPermissionManager::GetHost(nsIURI * aURI=0x00000000, 
nsACString & aResult={...})  Line 943 + 0x7	C++
>	cookie.dll!nsPermissionManager::TestPermission(nsIURI * 
aURI=0x00000000, const char * aType=0x0256e9f8, unsigned int * 
aPermission=0x0012df34)  Line 378	C++
 	xpcom.dll!XPTC_InvokeByIndex(nsISupports * that=0x01a4b908, unsigned 
int methodIndex=0x00000006, unsigned int paramCount=0x00000003, nsXPTCVariant * 
params=0x0012df14)  Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, 
XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2028 + 0x16	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x00dbebb8, JSObject * 
obj=0x02a933f8, unsigned int argc=0x00000002, long * argv=0x02652394, long * 
vp=0x0012e174)  Line 1287 + 0xa	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x10033ccb, unsigned int 
argc=0x01a4b908, unsigned int flags=0x00000000)  Line 1281 + 0x11	C
 	js3250.dll!js_Interpret(JSContext * cx=0x01a4b908, long * 
result=0x00000000)  Line 3371	C
 	js3250.dll!js_Invoke(JSContext * cx=0x10033ccb, unsigned int 
argc=0x01a4b908, unsigned int flags=0x00000000)  Line 1301 + 0xa	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x00dbebe4, JSObject * 
obj=0x029849e0, long fval=0x02a933d0, unsigned int flags=0x00000000, unsigned 
int argc=0x00000001, long * argv=0x0012e594, long * rval=0x0012e5b8)  Line 1378 
+ 0xe	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x00dbebb8, JSObject * 
obj=0x029849e0, long fval=0x02a933d0, unsigned int argc=0x00000001, long * 
argv=0x0012e594, long * rval=0x0012e5b8)  Line 3631 + 0x1a	C
 	gklayout.dll!nsJSContext::CallEventHandler(JSObject * 
aTarget=0x029849e0, JSObject * aHandler=0x02a933d0, unsigned int 
argc=0x00000001, long * argv=0x0012e594, long * rval=0x0012e5b8)  Line 1278 + 
0x18	C++
 	gklayout.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * 
aEvent=0x029439d0)  Line 183 + 0x37	C++
 	gklayout.dll!nsEventListenerManager::HandleEventSubType
(nsListenerStruct * aListenerStruct=0x02413d14, nsIDOMEvent * 
aDOMEvent=0x029439d0, nsIDOMEventTarget * aCurrentTarget=0x02a49a50, unsigned 
int aSubType=0x029439dc, unsigned int aPhaseFlags=0x00e1a188)  Line 1460 + 0xb
	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsIPresContext * 
aPresContext=0x00000000, nsEvent * aEvent=0x0012eacc, nsIDOMEvent * * 
aDOMEvent=0x0012e9ac, nsIDOMEventTarget * aCurrentTarget=0x02a49a50, unsigned 
int aFlags=0x00000007, nsEventStatus * aEventStatus=0x0012ec54)  Line 1553 + 
0x21	C++
 	gklayout.dll!nsXULElement::HandleDOMEvent(nsIPresContext * 
aPresContext=0x01b84b40, nsEvent * aEvent=0x02a49a50, nsIDOMEvent * * 
aDOMEvent=0x0012e9ac, unsigned int aFlags=0x00000007, nsEventStatus * 
aEventStatus=0x0012ec54)  Line 2788	C++
 	gklayout.dll!PresShell::HandleDOMEventWithTarget(nsIContent * 
aTargetContent=0x01b44e20, nsEvent * aEvent=0x0012eacc, nsEventStatus * 
aStatus=0x0012ec54)  Line 6153	C++
 	gklayout.dll!nsMenuFrame::OnCreate()  Line 1735	C++
 	gklayout.dll!nsMenuFrame::OpenMenuInternal(int 
aActivateFlag=0x00000001)  Line 805 + 0x5	C++
 	gklayout.dll!nsMenuFrame::AttributeChanged(nsIPresContext * 
aPresContext=0x0012e0b4, nsIContent * aChild=0x0012def0, int 
aNameSpaceID=0x10033ccb, nsIAtom * aAttribute=0x01a4b908, int 
aModType=0x00000000)  Line 757	C++
 	gklayout.dll!nsCSSFrameConstructor::AttributeChanged(nsIPresContext * 
aPresContext=0x0012e0b4, nsIContent * aContent=0x0012def0, int 
aNameSpaceID=0x10033ccb, nsIAtom * aAttribute=0x01a4b908, int 
aModType=0x00000000)  Line 10110	C++
 	gklayout.dll!PresShell::AttributeChanged(nsIDocument * 
aDocument=0x023a0ff8, nsIContent * aContent=0x02427668, int 
aNameSpaceID=0x00000000, nsIAtom * aAttribute=0x00e1a318, int 
aModType=0x00000002)  Line 5263	C++
 	gklayout.dll!nsXULDocument::AttributeChanged(nsIContent * 
aElement=0x0012def0, int aNameSpaceID=0x10033ccb, nsIAtom * 
aAttribute=0x01a4b908, int aModType=0x00000000)  Line 1137 + 0x14	C++
 	gklayout.dll!nsXULElement::SetAttrAndNotify(int 
aNamespaceID=0x00000000, nsIAtom * aAttribute=0x00e1a318, nsIAtom * 
aPrefix=0x00000000, const nsAString & aOldValue={...}, nsAttrValue & 
aParsedValue={...}, int aModification=0x02000000, int aFireMutation=0x00000000, 
int aNotify=0x00000001)  Line 2172	C++
 	gklayout.dll!nsXULElement::SetAttr(int aNamespaceID=0x0012e0b4, nsIAtom 
* aName=0x0012def0, nsIAtom * aPrefix=0x10033ccb, const nsAString & aValue=
{...}, int aNotify=0x00000000)  Line 2095 + 0x1f	C++
 	gklayout.dll!nsXULElement::SetAttribute(const nsAString & aName={...}, 
const nsAString & aValue={...})  Line 1020 + 0xf	C++
 	gklayout.dll!nsMenuFrame::OpenMenu(int aActivateFlag=0x00000001)  Line 
788 + 0x40	C++
 	gklayout.dll!nsMenuBarFrame::ShortcutNavigation(nsIDOMKeyEvent * 
aKeyEvent=0x02a49040, int & aHandledFlag=0x00000001)  Line 378	C++
 	gklayout.dll!nsMenuBarListener::KeyPress(nsIDOMEvent * 
aKeyEvent=0x02a49048)  Line 232	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsIPresContext * 
aPresContext=0x029c5158, nsEvent * aEvent=0x0012f99c, nsIDOMEvent * * 
aDOMEvent=0x0012f774, nsIDOMEventTarget * aCurrentTarget=0x023a1098, unsigned 
int aFlags=0x00000002, nsEventStatus * aEventStatus=0x0012f90c)  Line 1546 + 
0x29	C++
 	gklayout.dll!nsXULDocument::HandleDOMEvent(nsIPresContext * 
aPresContext=0x01b84b40, nsEvent * aEvent=0x0012f99c, nsIDOMEvent * * 
aDOMEvent=0x0012f774, unsigned int aFlags=0x00000002, nsEventStatus * 
aEventStatus=0x0012f90c)  Line 1239	C++
 	gklayout.dll!nsXULElement::HandleDOMEvent(nsIPresContext * 
aPresContext=0x01b84b40, nsEvent * aEvent=0x0012f99c, nsIDOMEvent * * 
aDOMEvent=0x0012f774, unsigned int aFlags=0x00000007, nsEventStatus * 
aEventStatus=0x0012f90c)  Line 2811 + 0x1c	C++
 	gklayout.dll!PresShell::HandleEventInternal(nsEvent * 
aEvent=0x02359e80, nsIView * aView=0x02379d90, unsigned int aFlags=0x00000001, 
nsEventStatus * aStatus=0x0012f90c)  Line 6073 + 0x11	C++
 	gklayout.dll!PresShell::HandleEvent(nsIView * aView=0x02379d90, 
nsGUIEvent * aEvent=0x0012f99c, nsEventStatus * aEventStatus=0x0012f90c, int 
aForceHandle=0x00000001, int & aHandled=0x00000001)  Line 5966 + 0x11	C++
 	gklayout.dll!nsViewManager::HandleEvent(nsView * aView=0x10033ccb, 
nsGUIEvent * aEvent=0x01a4b908, int aCaptured=0x00000000)  Line 2153	C++
 	gklayout.dll!nsViewManager::DispatchEvent(nsGUIEvent * 
aEvent=0x3d888889, nsEventStatus * aStatus=0x0012f960)  Line 1939 + 0x14
	C++
 	gklayout.dll!HandleEvent(nsGUIEvent * aEvent=0x0012f99c)  Line 79
	C++
 	gkwidget.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0012f99c, 
nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 1067 + 0x3	C++
 	gkwidget.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * 
event=0x00000000)  Line 1088	C++
 	gkwidget.dll!nsWindow::DispatchKeyEvent(unsigned int 
aEventType=0x00000083, unsigned short aCharCode=0x0074, unsigned int 
aVirtualCharCode=0x00000000, long aKeyData=0x00000000)  Line 2954 + 0xe	C++
 	gkwidget.dll!nsWindow::OnChar(unsigned int charCode=0x00000074)  Line 
3113 + 0x11	C++
 	gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=0x00000106, 
unsigned int wParam=0x00000074, long lParam=0x00000000, long * 
aRetValue=0x0012fc88)  Line 3804 + 0xa	C++
 	gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x003413e0, unsigned 
int msg=0x00000106, unsigned int wParam=0x00000074, long lParam=0x01bb4f34)  
Line 1349 + 0x10	C++
 	user32.dll!77d43a50() 	
 	user32.dll!77d43b1f() 	
 	user32.dll!TranslateMessage()  + 0xef	
 	user32.dll!GetMessageW()  + 0x125	
 	user32.dll!DispatchMessageW()  + 0xb	
 	appshell.dll!nsAppShellService::Run()  Line 524	C++
 	mozilla.exe!main1(int argc=0x01a4b908, char * * argv=0x00000000, 
nsISupports * nativeApp=0x0012de74)  Line 1302 + 0x9	C++
 	mozilla.exe!main(int argc=0x00000001, char * * argv=0x002a4190)  Line 
1779 + 0x16	C++
 	mozilla.exe!WinMain(HINSTANCE__ * __formal=0x00400000, HINSTANCE__ * 
__formal=0x00400000, char * args=0x00152324, HINSTANCE__ * 
__formal=0x00400000)  Line 1807 + 0x17	C++
 	mozilla.exe!WinMainCRTStartup()  Line 392 + 0xf	C
 	kernel32.dll!GetCurrentDirectoryW()  + 0x44	

Since this is a scriptable nsimethod, the code needs to actually do null checks 
instead of simply asserting.

(there would be an assert in a debug build, but i'm not using a debug build).
(Assignee)

Comment 1

14 years ago
Created attachment 148290 [details] [diff] [review]
(Av1) patch

that should do it
Assignee: darin → db48x
Status: UNCONFIRMED → ASSIGNED
Comment on attachment 148290 [details] [diff] [review]
(Av1) patch

>+  NS_ENSURE_ARG_POINTER(aURI || aType);

This will expand to |if(!(aURI || aType))|, so will only return if both are
null. You could use &&, but don't. Just make it two checks.
Attachment #148290 - Flags: review-
(Assignee)

Comment 3

14 years ago
mvl: so it will. consider it done.
(Assignee)

Comment 4

14 years ago
Created attachment 148336 [details] [diff] [review]
(Av2) patch
Attachment #148290 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #148336 - Flags: review?(mvl)
Comment on attachment 148336 [details] [diff] [review]
(Av2) patch

> nsPermissionManager::GetHost(nsIURI *aURI, nsACString &aResult)
> {
>-  NS_ASSERTION(aURI, "could not get uri");
>+  NS_ENSURE_ARG_POINTER(aURI);

You can remove that check. This internal function is only called after the
other checks are done, so you know aURI isn't null.
r=mvl with that changed.

(next time, please make the patch have some more context and use -p, makes it
easier to review)
Attachment #148336 - Flags: review?(mvl) → review+
Attachment #148290 - Attachment description: patch → (Av1) patch
Attachment #148336 - Attachment description: patch → (Av2) patch
Attachment #148336 - Attachment is obsolete: true
Created attachment 149136 [details] [diff] [review]
(Av2b) <nsPermissionManager.cpp> [Check in: see comment 42]

Av2, with comment 5 suggestion(s).
Comment on attachment 149136 [details] [diff] [review]
(Av2b) <nsPermissionManager.cpp> [Check in: see comment 42]

Keeping
{{
(Av2) patch	 patch		2004-05-12 13:26 PDT	mvl: review+
}}
Attachment #149136 - Flags: superreview?(darin)
Attachment #149136 - Flags: review+
OS: Windows XP → All
Hardware: PC → All
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7) Gecko/20040514] (<-- 1.7rc2 !)
(W98SE)

(For the/my record)
I found this bug after looking for
{{
Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIControllers.removeController]"  nsresult:
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://navigator/content/navigator.js :: Shutdown :: line 736"  data: no]
}}
which I got while starting Browser from my Browser+Console desktop icon while
having QuickLaunch loaded.

Comment 9

14 years ago
The assertions are there to tell programmers that they have done a no-no. 
Runtime checks are costly, do we really need them here?
(Reporter)

Comment 10

14 years ago
darin: things that are exposed to js should not crash. if you want a faster
internal implementation which is used by callers that are known to be sane, then
feel free to bloat the code with a faster unsafe version.
(Reporter)

Comment 11

14 years ago
note that this is an extension in code which is hardly performance critical. i
don't even build this extension most of the time.
The fact that you don't need it to have mozilla running doesn't make it not
performance critical.
The function is called for every image load, and in the future for every
pageload, image load, plugin load, stylesheet, everything.

And I kind of agree with darin here. This is called from chrome. Chrome can do
all kinds of nasty things, and should be safe code. It may be written in js, but
that doesn't make it untrusted. However, i won't stop the patch from getting in.

Comment 13

14 years ago
Comment on attachment 149136 [details] [diff] [review]
(Av2b) <nsPermissionManager.cpp> [Check in: see comment 42]

i think this patch is unnecessary.  the bug is invalid IMO.

garbage in => garbage out

if we add null checks here, should we also add null checks to the countless
other xpcom method implementations that are lacking null checks?  that spells
code bloat.  is this really something that needs to be so robust as to protect
against programmer error?  programmers can make other kinds of nasty errors
too.

if you can tell me that this is causing some actual deployment to crash, then
i'll reconsider, but otherwise, this seems like the wrong thing in general.
Attachment #149136 - Flags: superreview?(darin) → superreview-
Darin: would bug 277867 count as crashing in a deployment? talkback-public shows
92 crashes in nsPermissionManager::GetHost right now, looking like this same
thing to my uneducated eye. Though maybe the comment in
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=2983806
has the right idea.
(Reporter)

Updated

13 years ago
Keywords: topcrash
Darin, there's a difference between testing XPCOM out params (which I agree is
not needed) and testing in params, especially ones that can be passed from JS,
for validity  We seem to have this sorta-rule of "JS shouldn't crash".

Also, note that the api in question never clearly documents that a null URI is
unacceptable, so crashing seems particularly harsh...
can we maybe add a [notnull] IDL attribute for parameters, that xpconnect
enforces? that way, the various impls do not need all the nullchecks, but people
who think JS should never crash are still happy.
(Reporter)

Comment 17

13 years ago
SURE, we can make xpconnect dispatch even *slower*. let's not.
a JSVAL_IS_NULL for each parameter shouldn't be slow, and you only need to check
the attribute if the val is null
timeless: it's gonna be slow either way, and more code bloat to distribute the
null checks.  The extra cost of checking a typelib flag shouldn't amount to a
hill o' beans in this crazy world.  Cc'ing dbradley (David, see comment 16;
cc'ing shaver for his thoughts too).

/be

Comment 20

13 years ago
Has anyone figured out which consumer is passing null?  Clearly the consumer is
in a bad state if it is passing null to this API.  Adding the null checks on
(aURI) here might be the right thing to do since we have so many recorded
crashes in talkback, but the patch is also going to wallpaper over the real
problem, no?

Comment 21

13 years ago
> Also, note that the api in question never clearly documents that a null URI 
> is unacceptable, so crashing seems particularly harsh...

Well, it should be pretty clear from the API that a nsIURI is required for
anything to be done by the permission manager.  Clearly, there's no point
passing null, so clearly the consumer is confused... headed down a bad path,
going to get a bogus result, which could screw up something else anyways.  maybe
the thrown exception would cause some major feature of the browser to fail. 
sure, that is better than crashing, but at least now we know about that bug, and
maybe we can track it down and fix it.
personally I'd consider anyone passing null to something that does not
explicitly allow it to be broken.

Comment 23

13 years ago
The not-null will give a false sense of security for native to native calls.
Maybe it's ok if documented, but it's only going to apply for calls involving
XPConnect.

I'm also concerned about the number of flags we have left. We have three bits
left for parameters.
Created attachment 170969 [details] [diff] [review]
(Bv1) <nsPermissionManager.cpp> (cleanup only, no bug fix)

Av2b, keeping comment 5 suggestion(s) only.

Keeping
{{
(Av2) patch	 patch		2004-05-12 13:26 PDT	mvl: review+
}}
Attachment #170969 - Flags: superreview?(darin)
Attachment #170969 - Flags: review+
dbradley: good point, I think we shouldn't break symmetry between JS=>C++ and
C++=>C++ calls into an interface method.

/be
Comment on attachment 170969 [details] [diff] [review]
(Bv1) <nsPermissionManager.cpp> (cleanup only, no bug fix)

What's the use of random whitespace changes?
Attachment #170969 - Flags: review+ → review-
(In reply to comment #26)
> (From update of attachment 170969 [details] [diff] [review] [edit])
> What's the use of random whitespace changes?

Not that random: To improve readability !? (my point of view)
Are you interested in the code line removal at least ?
Comment on attachment 170969 [details] [diff] [review]
(Bv1) <nsPermissionManager.cpp> (cleanup only, no bug fix)

why remove the assertion?
(In reply to comment #28)
> (From update of attachment 170969 [details] [diff] [review] [edit])
> why remove the assertion?

Comment 5 says
"You can remove that check. This internal function is only called after the
other checks are done, so you know aURI isn't null."

And this is right at the 2 call lines in this file:
I seems that this (3rd) assertion can't be triggered as it is, == useless.

Comment 30

13 years ago
Comment on attachment 170969 [details] [diff] [review]
(Bv1) <nsPermissionManager.cpp> (cleanup only, no bug fix)

ask again when you have r=
Attachment #170969 - Flags: superreview?(darin)
(In reply to comment #29)
> I seems that this (3rd) assertion can't be triggered as it is, == useless.

well, each time you do trigger an assertion, there is a bug... I'm just thinking
that this assert might help catch/debug some bugs.
(In reply to comment #31)
> (In reply to comment #29)
> > I seems that this (3rd) assertion can't be triggered as it is, == useless.
> 
> well, each time you do trigger an assertion, there is a bug... I'm just thinking
> that this assert might help catch/debug some bugs.

Michiel pointed it out, and I agreed:
it seems that to get this assertion, you have to get the exact same one from one
of the two caller place first: little need to get it twice !?

Yet, if you prefer to keep it, that's fine with me :-|
Let's just keep this bug for actaully fixing the problem, not for random cleanup.

about that: Do we have any idea what caller is passing in null? On a quick
search, i couldn't find any obvious wrong callers.

Comment 34

13 years ago
Didn't have time to check, but a quick question. Has the interface changed? If 
it has, it could be old code hitting the newer interface and getting the wrong 
method.
Last change was 2003-07-08, so seems unlikely. (that change didn't change the
iid though. oops)
No way of knowing if this is the only one, but the bug that led me here, bug
277867, fingered the Permit Cookie extension (I'd guess,
http://extensionroom.mozdev.org/more-info/permitcookies) as the cause of his
crashes.

Updated

13 years ago
Blocks: 276895
Attachment #149136 - Attachment is obsolete: true
Comment on attachment 170969 [details] [diff] [review]
(Bv1) <nsPermissionManager.cpp> (cleanup only, no bug fix)

(In reply to comment #33)
> Let's just keep this bug for actaully fixing the problem, not for random cleanup.

As you wish :-/
Attachment #170969 - Attachment is obsolete: true
Comment on attachment 148336 [details] [diff] [review]
(Av2) patch

sr=bzbarsky.  If people want to add asserts too to make this easier to hunt in
debug builds, go for it.
Attachment #148336 - Flags: superreview+
(In reply to comment #38)
> (From update of attachment 148336 [details] [diff] [review] [edit])
> sr=bzbarsky.  If people want to add asserts too to make this easier to hunt in
> debug builds, go for it.

I'm lost here:
now Boris sr+ patch Av2,
but Michiel used to want Av2b, (but r- Bv1 later)
which Darin sr- then.

Could you three agree explicitly on which of these three obsoleted patchs to
revive and check in ? Or is anyone of you to decide for the others ?
(My choice would be Av2b, if it matters.)

Thanks.
timeless, could you please note which patch you checked in, and why the whole
discussion on if it was a good thing was ignored?
(Reporter)

Comment 41

13 years ago
i checked in a hand editted version of some patch. the discussion was ignored
because it was a waste of time. i regret not writing the patch myself and
getting bz to r+sr the patch.

mozilla/extensions/cookie/nsPermissionManager.cpp 	1.53

the stamp is basically against the bug and some random patch to be named later.
continuing to crash because of politics and perfection was silly. for reference,
bz's sr essentially replaces darin's objection because darin was wrong and
accepted that.

i'm actually resolving this bug instead of my normal practice of just annotating
bugs, because i filed this bug, know exactly why it was filed, and fully believe
that the changes i committed will fix this bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Comment on attachment 149136 [details] [diff] [review]
(Av2b) <nsPermissionManager.cpp> [Check in: see comment 42]

(In reply to comment #41)
> i checked in a hand editted version of some patch.
> 
> mozilla/extensions/cookie/nsPermissionManager.cpp 	1.53

For the record, it's 'Av2b',
with |NS_ASSERTION(aPermission, "no permission pointer");| _removed_ from
|TestPermission()|.
Attachment #149136 - Attachment description: (Av2b) <nsPermissionManager.cpp> → (Av2b) <nsPermissionManager.cpp> [Check in: see comment 42]

Comment 43

13 years ago
> the discussion was ignored because it was a waste of time.

This comment is quite disrespectful and IMO reckless.


> i regret not writing the patch myself and getting bz to r+sr the patch.

I think the owners of the to-be-patched module have the right to question any
changes to their module.

What response do you have to the issues I raised in comment #20 and #21?


I can only assume that something was discussed about this bug on IRC.  Perhaps
you and Boris came to some reasonable conclusion about this bug.  Perhaps you
uncovered who the bad JS consumer is and determined that this is the best course
given the circumstances.  At any rate, I'm not sure what the rush was or why you
chose not to discuss things here... it's not like this fix was going to miss a
release.  Oh well.
timeless: you are way out of line.  Instead of grandstanding and playing the
martyr, recap the conversation(s) you had on IRC.

Someone identify the bad caller or I'm gonna reopen this bug and sit on whoever
closed it last to get a resolution that wins the module owner's approval.

/be
Given the opening of comment 0, I bet the browser.xml in question was returning
null for currentURI and not throwing an exception in the process (which seems
well within the possibilities for .currentURI given the totally non-existent
documentation on what it should be, btw).

There are dozens of JS callers that pass the currentURI of a browser to
permission manager with nary a test to see whether it's non-null...

Now I do think that relying on the .currentURI behavior of throwing if there
isn't one is reasonable (and should be documented in browser.xml).

I also still think that unless we very clearly document that this api crashes if
the URI passed in is null, we should be null-checking it.

I'm fine with no null-check if the possible crash on null URI is clearly
documented as part of the API.

These last two paragraphs basically sum up my side of the IRC conversation in
question.  I wasn't quite expecting patches to be checked in without ok from
module owners...
Target Milestone: --- → mozilla1.8beta
(Reporter)

Comment 46

13 years ago
*** Bug 277867 has been marked as a duplicate of this bug. ***
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b) Gecko/20050121] (nightly) (W98SE)

The reported <navigator.js> exception remains: see bug 243894.
Crash Signature: [@ nsPermissionManager::GetHost]
You need to log in before you can comment on or make changes to this bug.