Closed Bug 1287623 Opened 3 years ago Closed 3 years ago

Use StaticRefPtr<> in more places

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

Details

Attachments

(5 files)

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.
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.
Attachment #8772568 - Flags: review?(nfroyd)
Attachment #8772568 - Flags: review?(nfroyd) → review+
Attachment #8772567 - Flags: review?(nfroyd) → review+
Attachment #8772569 - Flags: review?(honzab.moz) → review?(michal.novotny)
Attachment #8772570 - Flags: review?(honzab.moz) → review?(michal.novotny)
Attachment #8772569 - Flags: review?(michal.novotny) → review+
Attachment #8772570 - Flags: review?(michal.novotny) → review+
Attachment #8772566 - Flags: review?(bugs) → review+
Pushed by amccreight@mozilla.com:
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
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.
Flags: needinfo?(continuation) → needinfo?(nfroyd)
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.
Flags: needinfo?(nfroyd)
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().
Pushed by amccreight@mozilla.com:
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
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
You need to log in before you can comment on or make changes to this bug.