285461 - Permission manager assert on startup

Status: RESOLVED WORKSFORME

(Core :: Networking: Cookies, defect)

Description

[Mike Kaply [:mkaply]]

I was annoyed by the permission manage assert on startup and decided to debug it.

Problem is twofold.

1. On Windows and OS/2, OpenNSPRFileDesc should bail early if the file isn't
found and PR_RDONLY was requested. It should not try to open the file.

2. The NS_ENSURE_SUCCESS in nsPermissionMAnager.cpp should be returns.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Fix for problem

Fix OS/2 and Windows NSPRFileDesc code.

Above mentioned change to permissionmanager

Comment 2

[Darin Fisher]

Comment on attachment 176908 [details] [diff] [review]
Fix for problem

>Index: nsLocalFileOS2.cpp

>+    if (NS_FAILED(rv))
>+       if ((rv != NS_ERROR_FILE_NOT_FOUND) || ((rv == NS_ERROR_FILE_NOT_FOUND) && (flags = PR_RDONLY)))
>+           return rv; 

(flags = PR_RDONLY) is probably not what you meant since it will always
be true.  I think you meant "flags == PR_RDONLY", but I think even that
is not quite right.  I think you want something like this:

  ((flags & (PR_RDONLY | PR_WRONLY | PR_RDWR)) == PR_RDONLY)

BTW, isn't this needed on other platforms?


>Index: nsPermissionManager.cpp

>     rv = NS_NewLocalFileInputStream(getter_AddRefs(fileInputStream), oldPermissionsFile);
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    if (NS_FAILED(rv))
>+      return NS_OK;   

When translating an error condition into a success code, it is good to
leave a comment explaining why that is a good idea :-)


>   nsCOMPtr<nsILineInputStream> lineInputStream = do_QueryInterface(fileInputStream, &rv);
>+  if (NS_FAILED(rv))
>+    return NS_OK;   

ditto

Comment 3

[Mike Kaply [:mkaply]]

Mac actually did the right thing:

http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileMac.cpp#1192

Maybe I should use this as a guideline.

OSX doesn't do anything special with FILE_NOT_FOUND, so it should be OK:

http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileOSX.cpp#1155

Unix doesn't even stat the file first

http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileUnix.cpp#371

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

why is trying to open a nonexistant file not an error?

Comment 5

[Mike Kaply [:mkaply]]

(In reply to comment #4)
> why is trying to open a nonexistant file not an error?

Are you referring to the permission manager change or the NSPRFileDesc change?

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

To OpenNSPRFileDesc, sorry for not making that clear.

Comment 7

[Darin Fisher]

-> default owner

Comment 8

[[e-mail]]

the permmgr code in question has been fixed, and the localfile code looks to have changed, so i'm not sure this bug is relevant anymore (and if it was, it belongs in xpcom, not cookies). closing this out as WFM; please reopen and move components if necessary.