Closed
Bug 243385
Opened 20 years ago
Closed 20 years ago
Crash [@ nsPermissionManager::GetHost]
Categories
(Core :: Networking: Cookies, defect)
Core
Networking: Cookies
Tracking
()
RESOLVED
FIXED
mozilla1.8beta1
People
(Reporter: timeless, Assigned: db48x)
References
Details
(Keywords: assertion, crash, topcrash)
Crash Data
Attachments
(4 obsolete files)
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•20 years ago
|
||
that should do it
Assignee: darin → db48x
Status: UNCONFIRMED → ASSIGNED
Comment 2•20 years ago
|
||
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•20 years ago
|
||
mvl: so it will. consider it done.
Assignee | ||
Comment 4•20 years ago
|
||
Attachment #148290 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #148336 -
Flags: review?(mvl)
Comment 5•20 years ago
|
||
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+
Updated•20 years ago
|
Attachment #148290 -
Attachment description: patch → (Av1) patch
Updated•20 years ago
|
Attachment #148336 -
Attachment description: patch → (Av2) patch
Attachment #148336 -
Attachment is obsolete: true
Comment 7•20 years ago
|
||
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+
Updated•20 years ago
|
OS: Windows XP → All
Hardware: PC → All
Comment 8•20 years ago
|
||
[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•20 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•20 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•20 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.
Comment 12•20 years ago
|
||
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•20 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-
Comment 14•20 years ago
|
||
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.
Comment 15•20 years ago
|
||
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...
Comment 16•20 years ago
|
||
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•20 years ago
|
||
SURE, we can make xpconnect dispatch even *slower*. let's not.
Comment 18•20 years ago
|
||
a JSVAL_IS_NULL for each parameter shouldn't be slow, and you only need to check the attribute if the val is null
Comment 19•20 years ago
|
||
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•20 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•20 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.
Comment 22•20 years ago
|
||
personally I'd consider anyone passing null to something that does not explicitly allow it to be broken.
Comment 23•20 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.
Comment 24•20 years ago
|
||
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+
Comment 25•20 years ago
|
||
dbradley: good point, I think we shouldn't break symmetry between JS=>C++ and C++=>C++ calls into an interface method. /be
Comment 26•20 years ago
|
||
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-
Comment 27•20 years ago
|
||
(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 28•20 years ago
|
||
Comment on attachment 170969 [details] [diff] [review] (Bv1) <nsPermissionManager.cpp> (cleanup only, no bug fix) why remove the assertion?
Comment 29•20 years ago
|
||
(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•20 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)
Comment 31•20 years ago
|
||
(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.
Comment 32•20 years ago
|
||
(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 :-|
Comment 33•20 years ago
|
||
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•20 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.
Comment 35•20 years ago
|
||
Last change was 2003-07-08, so seems unlikely. (that change didn't change the iid though. oops)
Comment 36•20 years ago
|
||
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•20 years ago
|
Attachment #149136 -
Attachment is obsolete: true
Comment 37•20 years ago
|
||
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 :-/
Updated•20 years ago
|
Attachment #170969 -
Attachment is obsolete: true
Comment 38•20 years ago
|
||
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+
Comment 39•20 years ago
|
||
(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.
Comment 40•20 years ago
|
||
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•20 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
Closed: 20 years ago
Resolution: --- → FIXED
Comment 42•20 years ago
|
||
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•20 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.
Comment 44•20 years ago
|
||
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
Comment 45•20 years ago
|
||
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...
Updated•20 years ago
|
Target Milestone: --- → mozilla1.8beta
Reporter | ||
Comment 46•20 years ago
|
||
*** Bug 277867 has been marked as a duplicate of this bug. ***
Comment 47•20 years ago
|
||
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b) Gecko/20050121] (nightly) (W98SE) The reported <navigator.js> exception remains: see bug 243894.
Updated•13 years ago
|
Crash Signature: [@ nsPermissionManager::GetHost]
You need to log in
before you can comment on or make changes to this bug.
Description
•