Closed Bug 940765 Opened 7 years ago Closed 7 years ago

Annotate pref_HashTableLookup as never GCing

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: sfink, Assigned: sfink)

References

Details

(Whiteboard: [qa-])

Attachments

(3 files, 2 obsolete files)

Hazard:

Function 'uint8 mozilla::dom::Navigator::HasTelephonySupport(JSContext*, JSObject*)' has unrooted 'aGlobal' of type 'JSObject*' live across GC call 'uint32 mozilla::Preferences::GetBool(int8*, uint8*)' at dom/base/Navigator.cpp:1702
    dom/base/Navigator.cpp:1701: Assign(1,2, enabled := 0)
    dom/base/Navigator.cpp:1702: Call(2,3, GetBool("dom.telephony.enabled",enabled))
    dom/base/Navigator.cpp:1703: Call(3,4, __temp_1 := __builtin_expect(!enabled*,0))
    dom/base/Navigator.cpp:1703: Assume(4,7, (__temp_1* != 0), false)
    dom/base/Navigator.cpp:1705: Call(7,8, __temp_3 := GetWindowFromGlobal(aGlobal*))
GC Function: uint32 mozilla::Preferences::GetBool(int8*, uint8*)
    PREF_GetBoolPref
    PrefHashEntry* pref_HashTableLookup(void*)
    PL_DHashTableOperate
    FieldCall: PLDHashTableOps.initEntry

This is a false positive. The analysis sees the call through an initEntry function pointer and freaks out, thinking it could call something that can GC. We could instruct the analysis to ignore this function, but it's a little nicer to put it in the code as documentation (and a dynamic verification in DEBUG mode.)
Blocks: 898606
Attachment #8334945 - Flags: review?(benjamin)
Comment on attachment 8334945 [details] [diff] [review]
Annotate pref_HashTableLookup as never GCing

Nope, this doesn't work. This tries to fetch a JSRuntime from TLS, and it's either on the wrong thread or there isn't one around to fetch yet.
Attachment #8334945 - Flags: review?(benjamin)
Attached patch pref_HashTableLookup cannot GC (obsolete) — Splinter Review
Seems easiest to annotate it in the analysis instead. :(
Attachment #8335137 - Flags: review?(terrence)
Attachment #8334945 - Attachment is obsolete: true
For the record, js::TlsPerThreadData.get() was returning NULL. So this was probably just firing too early?
Comment on attachment 8335137 [details] [diff] [review]
pref_HashTableLookup cannot GC

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

r=me
Attachment #8335137 - Flags: review?(terrence) → review+
Sadly, there is another way preferences::GetBool thinks it can GC: it calls InitStaticMembers, which goes through a FieldCall. At least in our system, we use the preferences to start up JS, so we're certainly not going to run JS code to figure out prefs.

I don't know if including things in js/... is kosher here, though.
Attachment #8335568 - Flags: review?(benjamin)
Attachment #8335137 - Attachment is obsolete: true
Depends on: 941262
Whiteboard: [leave open]
Landed a temporary annotation for InitStaticMembers. Will remove it if the AutoAssertNoGC patch gets r+.
Comment on attachment 8335568 [details] [diff] [review]
Preferences::InitStaticMembers does not GC in our world

The assert is incorrect. The prefservice can and does use JS via autoconfig files in Preferences::Init.
Attachment #8335568 - Flags: review?(benjamin) → review-
If it can GC, then we'd better stop claiming that it cannot.
Attachment #8345621 - Flags: review?(terrence)
The hazard would seem to be real, then. Sprinkle some rooting around to fix it.
Attachment #8346058 - Flags: review?(benjamin)
Comment on attachment 8345621 [details] [diff] [review]
preference service can GC

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

r=me
Attachment #8345621 - Flags: review?(terrence) → review+
Duplicate of this bug: 949887
Attachment #8346058 - Flags: review?(benjamin) → review?(bzbarsky)
Duplicate of this bug: 949888
Comment on attachment 8346058 [details] [diff] [review]
Root before preference checking

Fix the "unused" bits in the header too, and r=me
Attachment #8346058 - Flags: review?(bzbarsky) → review+
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.