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

RESOLVED FIXED in Firefox -esr60

Status

()

defect
P3
normal
RESOLVED FIXED
11 months ago
11 months ago

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)

(Assignee)

Description

11 months ago
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

11 months ago
Assignee: nobody → philip.chimento
Status: NEW → ASSIGNED

Updated

11 months ago
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

11 months ago
Thanks for the review! I wasn't sure if it was a bug or I was using the API incorrectly.
(Assignee)

Updated

11 months ago
Blocks: 1422930
No longer blocks: 1429930

Comment 4

11 months ago
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

11 months ago
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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d44967058ffa
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
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.