1287623 - Use StaticRefPtr<> in more places

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

I have patches to convert CacheFileIOManager::gInstance, CacheIndex::gInstance, nsDirectoryService::gService, and nsMessageManagerScriptExecutor::sScriptCacheCleaner to use StaticRefPtr<>. The first two will require adding some more methods involving StaticRefPtr.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Use StaticRefPtr for sScriptCacheCleaner.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e87c342a58d

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Use StaticRefPtr for nsDirectoryService::gService.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Add more methods involving StaticRefPtr.

This patch adds a number of standard conversions to and from RefPtr<> and already_AddRefed<>.

These are used in the later patches in this series.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Make CacheIndex::gInstance a StaticRefPtr.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 5 - Convert CacheFileIOManager::gInstance to StaticRefPtr.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/114da8bdc0b0
part 1 - Use StaticRefPtr for sScriptCacheCleaner. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/602d4d88e806
part 2 - Use StaticRefPtr for nsDirectoryService::gService. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa409c9b1ce
part 3 - Add more methods involving StaticRefPtr. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/248153344e15
part 4 - Make CacheIndex::gInstance a StaticRefPtr. r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/a48daec87ec9
part 5 - Convert CacheFileIOManager::gInstance to StaticRefPtr. r=mayhemer

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All backed out for static build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=32263507&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2c17679b215

Comment 8

[Andrew McCreight [:mccr8]]

Nathan, static analysis gave me the error message "error: variable of type 'mozilla::StaticRefPtr<mozilla::net::CacheFileIOManager>' only valid as global". Do you know why that isn't okay? Thanks.

I guess I'll just remove them from the class and turn them into globals in the C++ files.

Comment 9

[Andrew McCreight [:mccr8]]

Over IRC, glandium said this is due to the MOZ_ONLY_USED_TO_AVOID_STATIC_CONSTRUCTORS annotation, but AFAICT my use should be okay, because they are static members. I'll file a static analysis bug.

Comment 10

[Andrew McCreight [:mccr8]]

Oh, I see. The actual error is:

22:33:43     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:791:19: error: variable of type 'mozilla::StaticRefPtr<mozilla::net::CacheFileIOManager>' only valid as global
22:33:43     INFO -  NewRunnableMethod(PtrType aPtr, Method aMethod)
22:33:43     INFO -                    ^
22:33:43     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:791:19: note: value incorrectly allocated in an automatic variable
22:33:43     INFO -  1 error generated.

I suspect it means this line:
  ev = NewRunnableMethod(
    gInstance, &CacheFileIOManager::CacheIndexStateChangedInternal);

...and then some template magic is trying to declare something of type StaticRefPtr<>. I suspect the fix is to change that to gInstance.get().

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/872848c3a716
part 1 - Use StaticRefPtr for sScriptCacheCleaner. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/cef4132ee60b
part 2 - Use StaticRefPtr for nsDirectoryService::gService. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/aafe24d898d2
part 3 - Add more methods involving StaticRefPtr. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3676ef8ac27
part 4 - Make CacheIndex::gInstance a StaticRefPtr. r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f91720817ab
part 5 - Convert CacheFileIOManager::gInstance to StaticRefPtr. r=mayhemer

Comment 12

[Andrew McCreight [:mccr8]]

That get was the only missing thing, so I just updated that without bothering to get review.

clean S try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb3697ebcbfa

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/872848c3a716
https://hg.mozilla.org/mozilla-central/rev/cef4132ee60b
https://hg.mozilla.org/mozilla-central/rev/aafe24d898d2
https://hg.mozilla.org/mozilla-central/rev/c3676ef8ac27
https://hg.mozilla.org/mozilla-central/rev/2f91720817ab