1294659 - Cr is undefined in folderLookupService

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Kent James (:rkent)]

While trying to use folderLookupService, it threw an error, but go a secondary error because Cr is undefined.

Comment 1

[Kent James (:rkent)]

Attachment: define it

I believe that the bug is hit when you try to create a second instance of folderLookupService (which you should not do) and then the throw has an error.

Comment 2

[:aceman]

You mean the occurrence of Cr in this code?

function folderLookupService() {
  this._map = new Map();
  if (gCreated)
    throw Cr.NS_ERROR_ALREADY_INITIALIZED;
  gCreated = true;
}

The fix seems OK, the Cr is not defined in this file.

But do we want to kill this._map at the second (rejected) attempt? (Yes not caused by this bug...)

Comment 3

[Kent James (:rkent)]

"But do we want to kill this._map at the second (rejected) attempt?"

I'm not sure I understand the question. On the second attempt, a new "this" would be created, and this._map is new. I don't think that it matters if you reorder things.

But I don't really understand how .getService() (which is supposed to be used instead of .createInstance) maintains a single copy of the service.

Are you suggesting that the this._map = ... be moved to after the if (gCreated)?

Comment 4

[:aceman]

Yes, that was my idea.
But if that is a seconds object with a different this._map then all is fine, just it may a be a bit inefficient (creating a Map but then saying it is not to be used).

Comment 5

[:aceman]

So while you have the code to exercise the creation of the object, can you check if you get 2 different objects, or if it is a same one? gCreated being outside the object could indicate it tries to control multiple objects. However, if the second attempt really creates a new object, just throws, the error, what prevents the user to use that object? It is still fully initialized and no further checks are done on usage. Or does the exception somehow prevent getting reference to the new object?

Comment 6

[Kent James (:rkent)]

In my tests,

1) two calls to .getService return the same object

2) a call to .createInstance, which fails with a throw, does not set the value of the return variable (at least in my call from JS) even if I catch the error.

The assumption that I have always operated on with JS XPCOM components, which this method assumes is true, is that the JS for the component factory exists for the life of the process, in its own JS scope, so that global variables exist for the life of the program. This is the same behavior as Mozilla JS modules.

Comment 7

[:aceman]

So, the answer whether this._map is the same object in the 2 calls?

I tried it myself, by calling .createInstance in the middle of test_folderLookupService.js and it seems to me this._map was undefined in both of the cases (first is getService at the top of the test) at the start of "function folderLookupService()".
Also, after that point calling "do_check_eq(fls.getFolderForURL(kRootURI), root);" still works and the root folder is in the cache (so the .createInstance) has not wiped the map of the first object.

So it seems moving the this._map = new Map() to below the check of gCreated does not affect correctness, it would only improve speed (do not create a map that will never be used).
It is true that after the .createInstance call, the return value is null so the caller does not have a reference to the second object and can't call the methods. But if he somehow got valid second object, having this._map being null would throw exceptions when trying to use it as a map.

So, I think moving the new Map() to below check of gCreated would be useful and at least nobody would need to wonder about it in the future :)

Comment 8

[:aceman]

Comment on attachment 8780460 [details] [diff] [review]
define it

Review of attachment 8780460 [details] [diff] [review]:
-----------------------------------------------------------------

You can decide whether to make the change here, when we already discussed it. Or I can do it in a new bug as I see another problem in the folderlookup file anyway.

Comment 9

[Kent James (:rkent)]

http://hg.mozilla.org/comm-central/rev/3752045b179d