JS::GCHashMap<JS::Heap<JSString*>, ...> does not compile

RESOLVED FIXED in Firefox -esr60

Status

()

defect
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: ptomato, Assigned: ptomato)

Tracking

(Blocks 2 bugs)

60 Branch
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 fixed, firefox61 wontfix, firefox62 fixed)

Details

Attachments

(2 attachments)

Posted file Minimal example
This seems to be a regression in between ESR52 and ESR60. The attached example program compiles and runs with ESR52 (barring the JSClassOps layout change) but does not compile with ESR60:

In file included from bug.cpp:5:
In file included from .../include/mozjs-60/jsapi.h:29:
In file included from .../include/mozjs-60/js/CallArgs.h:73:
.../include/mozjs-60/js/RootingAPI.h:274:32: error: 
      no member named 'exposeToJS' in 'js::BarrierMethods<JSString *>'
        js::BarrierMethods<T>::exposeToJS(ptr);
                               ^
.../include/mozjs-60/js/RootingAPI.h:277:9: note: in
      instantiation of member function 'JS::Heap<JSString *>::exposeToActiveJS'
      requested here
        exposeToActiveJS();
        ^
.../include/mozjs-60/js/RootingAPI.h:268:5: note: in
      instantiation of member function 'JS::Heap<JSString *>::get' requested
      here
    DECLARE_POINTER_CONSTREF_OPS(T);
    ^
.../include/mozjs-60/js/RootingAPI.h:160:40: note: 
      expanded from macro 'DECLARE_POINTER_CONSTREF_OPS'
    operator const T&() const { return get(); ...
                                       ^
...l/include/mozjs-60/js/HashTable.h:1378:34: note: 
      in instantiation of member function 'JS::Heap<JSString *>::operator
      JSString *const &' requested here
        return HashPolicy::match(HashPolicy::getKey(e.get()), l);
                                 ^
.../include/mozjs-60/js/HashTable.h:1406:42: note: 
      in instantiation of member function
      'js::detail::HashTable<js::HashMapEntry<JS::Heap<JSString *>, void *>,
      js::HashMap<JS::Heap<JSString *>, void *, js::DefaultHasher<JSString *>,
      js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::match'
      requested here
        if (entry->matchHash(keyHash) && match(*entry, l)) {
                                         ^
.../include/mozjs-60/js/HashTable.h:1782:23: note: 
      in instantiation of member function
      'js::detail::HashTable<js::HashMapEntry<JS::Heap<JSString *>, void *>,
      js::HashMap<JS::Heap<JSString *>, void *, js::DefaultHasher<JSString *>,
      js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::lookup'
      requested here
        return AddPtr(lookup(l, keyHash, sCollisionBit), *this, keyHash);
                      ^
.../include/mozjs-60/js/HashTable.h:152:21: note: in
      instantiation of member function
      'js::detail::HashTable<js::HashMapEntry<JS::Heap<JSString *>, void *>,
      js::HashMap<JS::Heap<JSString *>, void *, js::DefaultHasher<JSString *>,
      js::SystemAllocPolicy>::MapHashPolicy,
      js::SystemAllocPolicy>::lookupForAdd' requested here
        return impl.lookupForAdd(l);
                    ^
bug.cpp:46:15: note: in instantiation of member function
      'js::HashMap<JS::Heap<JSString *>, void *, js::DefaultHasher<JSString *>,
      js::SystemAllocPolicy>::lookupForAdd' requested here
      hashMap.lookupForAdd(str);
              ^

Adding the requested template specialization makes the program compile and run as expected.
Assignee

Updated

Last year
Assignee: nobody → philip.chimento
Status: NEW → ASSIGNED
Component: JavaScript Engine → JavaScript: GC
Priority: -- → P3
Comment on attachment 8981252 [details] [diff] [review]
Add exposeToActiveJS specialization for JSString

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

Great, thanks for fixing this.
Attachment #8981252 - Flags: review?(sphink) → review+
Assignee

Comment 3

Last year
Thanks for the review! I wasn't sure if it was a bug or I was using the API incorrectly.
Assignee

Updated

Last year
Blocks: 1422930
No longer blocks: 1429930

Comment 4

Last year
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d44967058ffa
Add exposeToActiveJS specialization for JSString. r=sfink
Keywords: checkin-needed
Assignee

Comment 5

Last year
Comment on attachment 8981252 [details] [diff] [review]
Add exposeToActiveJS specialization for JSString

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Allow a SpiderMonkey API to work as intended
User impact if declined: None to Firefox as the API isn't used within the codebase, but it's needed by embedders
Fix Landed on Version: 62
Risk to taking this patch (and alternatives if risky): Unlikely to cause any risk to Firefox, as any use of this API would previously not have compiled.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8981252 - Flags: approval-mozilla-esr60?

Comment 6

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/d44967058ffa
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment on attachment 8981252 [details] [diff] [review]
Add exposeToActiveJS specialization for JSString

Spidermonkey API fix for embedders. Approved for ESR 60.1. I'm not going to bother approving this for Beta61 since it seems unlikely to be particularly important there. Feel free to nominate the patch if I'm missing something, though.
Attachment #8981252 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.