114439 - nsFileTransportService should cache the mime service.

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[neeti]

Currently everytime we do a nsFileIO::Open(..), we call 
do_GetService(NS_MIMESERVICE_CONTRACTID,..)

Comment 1

[neeti]

Attachment: patch to cache mimeservice

Comment 2

[Suresh Duddi (gone)]

Comment on attachment 61106 [details] [diff] [review]
patch to cache mimeservice

When is the mInstance released ? If you haven't already found a way, 
netwerk/build/nsNetModule.cpp has a nsNeckoShutdown when it can be released.

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 61106 [details] [diff] [review]
patch to cache mimeservice

nsFileTransportService::GetInstance  should return a addref'ed interface
pointer.

Comment 4

[neeti]

Attachment: revised patch with release and addref of mInstance

Comment 5

[Suresh Duddi (gone)]

I see trouble - nsFileTransportService::GetInstance() is being called from both
nsFileTransportService::Create() - the thing that creates the instance and from
nsFileIO::Open() - the thing that just wants to use it. This model is an
addref/release nightmare esp if GetInstance() is addreffing.

Neeti and I talked and here is what Neeti is going to try:
- Eliminate GetInstance()
- Access and fill mInstance in Create()
- Get nsFileIO::Open() to check mInstance and call Create() if it aint created
already to get to the nsFileTransportService

Comment 6

[Darin Fisher]

wait a second!!

nsFileTransportService should set mInstance in its constructor, and clear it in
its destructor.

then nsFileTransportService::GetInstance() can be inlined.

then nsFileTransport should AddRef the FTS instance in its constructor and
Release the FTS instance in its destructor.

doing so ensures that the nsFileTransportService is alive for the entire
lifetime of a nsFileTransport.

the current implementation of GetInstance() is missing the fact that you can't
have a nsFileTransport without the nsFileTransportService already existing.  it
is the FTS that creates nsFileTransport objects.  so, there is no need to have
the check for if (!mInstance)

Comment 7

[neeti]

Attachment: revised patch with Darin's suggestions

Attached a new patch with Darins' comments.

Comment 8

[Suresh Duddi (gone)]

One of the things the earlier patch was doing was enforce singleton-ness of
nsFileTransportService, meaning, even if multiple calls to
CreateInstance(..filetransportservice..) happens, only one FTS will exist. That
isn't true anymore (and this is similar to the way code is now). Is that an issue ?

Comment 9

[Darin Fisher]

not an issue since everyone is expected to call do_GetService instead of
do_CreateInstance.  perhaps we should enforce it, but we don't here as well as
in many other places :(

i wouldn't object to adding code to enforce singleton-ness :)

Comment 10

[Darin Fisher]

Comment on attachment 61332 [details] [diff] [review]
revised patch with Darin's suggestions

>Index: src/nsFileStreams.cpp

>         }
>-	
>-        if (NS_FAILED(rv)) {
>+
>+ 	    if (!mimeServ || (NS_FAILED(rv))) {
>             // if all else fails treat it as text/html?

indentation problems.


>Index: src/nsFileTransportService.h
>+    nsCOMPtr<nsIMIMEService>    mMimeService;
>+    static nsFileTransportService*  mInstance;

alignment!

otherwise, sr=darin

Comment 11

[Suresh Duddi (gone)]

Comment on attachment 61332 [details] [diff] [review]
revised patch with Darin's suggestions

r=dp if we are absolutely sure that nsFileTransportService::mInstance is not
null when hitting nsFileIO::Open().

Comment 12

[neeti]

Attachment: fixed alignment and indentation

Comment 13

[Suresh Duddi (gone)]

No need to reattach patch for alignment stuff. Check it in. You have r=dp and
sr=darin

Comment 14

[Darin Fisher]

Comment on attachment 61370 [details] [diff] [review]
fixed alignment and indentation

sr=darin

Comment 15

[neeti]

fixed checked in. thanks everyone for the reviews.

Comment 16

[Leston Buell]

Could someone please write a summary line for this bug?