Closed
Bug 940765
Opened 11 years ago
Closed 11 years ago
Annotate pref_HashTableLookup as never GCing
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: sfink, Assigned: sfink)
References
Details
(Whiteboard: [qa-])
Attachments
(3 files, 2 obsolete files)
1.26 KB,
patch
|
benjamin
:
review-
|
Details | Diff | Splinter Review |
1.39 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
2.50 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #8334945 -
Flags: review?(benjamin)
Assignee | ||
Comment 2•11 years ago
|
||
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)
Assignee | ||
Comment 3•11 years ago
|
||
Seems easiest to annotate it in the analysis instead. :(
Attachment #8335137 -
Flags: review?(terrence)
Assignee | ||
Updated•11 years ago
|
Attachment #8334945 -
Attachment is obsolete: true
Assignee | ||
Comment 4•11 years ago
|
||
For the record, js::TlsPerThreadData.get() was returning NULL. So this was probably just firing too early?
Comment 5•11 years ago
|
||
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+
Assignee | ||
Comment 6•11 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/ab8b9448e062
Assignee | ||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8335137 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Whiteboard: [leave open]
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ab8b9448e062
Assignee | ||
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/326cf83b0416 https://hg.mozilla.org/integration/mozilla-inbound/rev/32ecfffd9509
Assignee | ||
Comment 10•11 years ago
|
||
Landed a temporary annotation for InitStaticMembers. Will remove it if the AutoAssertNoGC patch gets r+.
Comment 12•11 years ago
|
||
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-
Assignee | ||
Comment 13•11 years ago
|
||
If it can GC, then we'd better stop claiming that it cannot.
Attachment #8345621 -
Flags: review?(terrence)
Assignee | ||
Comment 14•11 years ago
|
||
The hazard would seem to be real, then. Sprinkle some rooting around to fix it.
Attachment #8346058 -
Flags: review?(benjamin)
Comment 15•11 years ago
|
||
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+
Assignee | ||
Comment 16•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8421a2088cc6
Assignee | ||
Updated•11 years ago
|
Attachment #8346058 -
Flags: review?(benjamin) → review?(bzbarsky)
Comment 19•11 years ago
|
||
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+
Assignee | ||
Comment 20•11 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/4b2d3255fb3b
Comment 21•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8421a2088cc6 https://hg.mozilla.org/mozilla-central/rev/4b2d3255fb3b
Assignee | ||
Updated•11 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla29
Updated•10 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•