Add JS::DefaultGlobalClassOps to JSAPI

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P2
normal
RESOLVED FIXED
8 months ago
3 months ago

People

(Reporter: ptomato, Assigned: ptomato)

Tracking

(Blocks 1 bug)

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 wontfix, firefox66 wontfix, firefox67 wontfix, firefox68 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Assignee

Description

8 months ago
Every "how to embed SpiderMonkey" example program currently begins with something like the following beast:

    static JSClassOps classOps = {
        nullptr,  // addProperty
        nullptr,  // deleteProperty
        nullptr,  // enumerate
        nullptr,  // newEnumerate
        nullptr,  // resolve
        nullptr,  // mayResolve
        nullptr,  // finalize
        nullptr,  // call
        nullptr,  // hasInstance
        nullptr,  // construct
        JS_GlobalObjectTraceHook
    };

We could just stick a copy of this in the public API. It's a better introduction to SpiderMonkey when you don't have to start out staring at something that is marginally relevant.
Assignee

Updated

8 months ago
Assignee: nobody → philip.chimento
Status: NEW → ASSIGNED
I wonder if DefaultGlobalClassOps is a better connotation than "simple".
Assignee

Updated

8 months ago
Summary: Add JSSimpleGlobalClassOps to JSAPI → Add JS::DefaultGlobalClassOps to JSAPI
Assignee

Comment 2

8 months ago
Agreed. Also I wasn't sure between JS::DefaultGlobalClassOps and JSDefaultGlobalClassOps so I went with the former.
I notice we still have js::ClassOps and JSClassOps. I wonder if we need both JS:DefaultGlobalClassOps and js::DefaultGlobalClassOps for now.

Also, probably good to split into two patches one to add them and one to use them. We can collect these ergonomic fixes and have a discussion about uplifts.
Blocks: sm.embedding
Assignee

Comment 4

8 months ago
This also adds a js::ClassOps variant, js::DefaultGlobalClassOps which can
be used in js::Class.

Depends on D11569
ptomato and I discussed this on IRC a few days ago. I'd like to see this land.
Flags: needinfo?(philip.chimento)
Priority: -- → P2
Assignee

Comment 7

7 months ago
Unfortunately, I haven't had time to work on the patches since this weekend.

I changed the JSClass to use lazy standard classes but I found on try that it breaks this test: https://searchfox.org/mozilla-central/source/js/src/jsapi-tests/testSetProperty.cpp#15

I just need to override getGlobalClass() in that test to use a global without lazy standard classes, but I probably won't be able to do that until next weekend.
The good news is, that comment is stale. We can now cache global property accesses -- at least the ones this test is doing -- I think. Please delete the assertion and the comment, but leave the test.
Assignee

Comment 9

7 months ago
I did find some time yesterday evening to update the patch, on D11571 ... because arc wasn't finding the hg revision I had to cat the patch directly to arc diff, so it didn't tag you automatically. I just tagged you now. The revision I have there just changes that test to use a global without lazy classes.

I can still undo that change and delete the assertion and comment, but it will likely be this weekend.
Assignee

Comment 10

7 months ago
Thanks for the review! So, the procedure now is that I attach the revisions from Phabricator to this bug again, and add checkin-needed?
Flags: needinfo?(philip.chimento)
Assignee

Comment 11

7 months ago
Reviewers: jorendorff

Subscribers: jandem

Bug #: 1506324

Differential Revision: https://phabricator.services.mozilla.com/D11570
Assignee

Updated

7 months ago
Attachment #9024222 - Attachment is obsolete: true
Assignee

Comment 12

7 months ago
Summary: Depends on D11570

Reviewers: tcampbell

Reviewed By: tcampbell

Subscribers: jandem

Bug #: 1506324

Differential Revision: https://phabricator.services.mozilla.com/D11571
Assignee

Updated

7 months ago
Attachment #9024223 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #9025832 - Flags: review+
Assignee

Updated

7 months ago
Attachment #9025833 - Flags: review+
Assignee

Updated

7 months ago
Keywords: checkin-needed
Attachment #9024222 - Attachment is obsolete: false

Comment 13

7 months ago
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7be95c95a3a7
Add JS::DefaultGlobalClassOps to JSAPI. r=jorendorff
Keywords: checkin-needed
Hi! The second part of the patch fails to apply cleanly. (Use DefaultGlobalClassOps in existing code)

This the rej file: 
--- tests.cpp
+++ tests.cpp
@@ -42,24 +36,10 @@ static JSObject*
 jsfuzz_createGlobal(JSContext* cx, JSPrincipals* principals)
 {
     /* Create the global object. */
-    JS::RootedObject newGlobal(cx);
     JS::RealmOptions options;
     options.creationOptions().setStreamsEnabled(true);
-    newGlobal = JS_NewGlobalObject(cx, getGlobalClass(), principals, JS::FireOnNewGlobalHook,
-                                   options);
-    if (!newGlobal) {
-        return nullptr;
-    }
-
-    JSAutoRealm ar(cx, newGlobal);
-
-    // Populate the global object with the standard globals like Object and
-    // Array.
-    if (!JS::InitRealmStandardClasses(cx)) {
-        return nullptr;
-    }
-
-    return newGlobal;
+    return JS_NewGlobalObject(cx, getGlobalClass(), principals, JS::FireOnNewGlobalHook,
+                              options);
 }
 
 static bool
Flags: needinfo?(philip.chimento)

Comment 15

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7be95c95a3a7
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
(In reply to Cosmin Sabou [:CosminS] from comment #14)
> Hi! The second part of the patch fails to apply cleanly. (Use
> DefaultGlobalClassOps in existing code)

Reopening for the second part.

(In reply to Philip Chimento [:ptomato] from comment #10)
> Thanks for the review! So, the procedure now is that I attach the revisions
> from Phabricator to this bug again, and add checkin-needed?

You can just set checkin-needed and sheriffs can land Phabricator patches :)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 17

7 months ago
Summary: Depends on D11570

Reviewers: tcampbell, jorendorff

Reviewed By: tcampbell, jorendorff

Subscribers: jandem

Bug #: 1506324

Differential Revision: https://phabricator.services.mozilla.com/D11571
Assignee

Updated

7 months ago
Attachment #9024222 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #9025833 - Attachment is obsolete: true
Flags: needinfo?(philip.chimento)
Assignee

Updated

7 months ago
Attachment #9026131 - Flags: review+
Attachment #9024223 - Attachment is obsolete: false
Assignee

Updated

7 months ago
Keywords: checkin-needed

Comment 18

7 months ago
Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f00f5277735f
Use DefaultGlobalClassOps in existing code. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/412920e602fa
Add JS::DefaultGlobalClassOps to JSAPI. r=ptomato
Keywords: checkin-needed
Assignee

Comment 20

7 months ago
Sorry I didn't get time to look at this in the past two weeks.

I think the failure is because D11570 was already merged, and then it was applied again. Only D11571 needs to be applied. But, I expect it won't apply any more because of the reformat, so I'll rebase it and upload it again.
Flags: needinfo?(philip.chimento)
The source tree is entering a soft-freeze before merging to Beta, so let us hold off until the start of next release. I don't think we have any hurry to get this anywhere. Feel free to ping me if you want some help untangling phabricator or find yourself too busy.
Assignee

Comment 22

7 months ago
No hurry indeed. I updated D11571, so it's in place for when the next cycle starts.

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:ptomato, could you have a look please?

Flags: needinfo?(philip.chimento)
Assignee

Updated

3 months ago
Flags: needinfo?(philip.chimento)
Keywords: checkin-needed
Assignee

Comment 24

3 months ago

Thanks for the reminder. D11571 needs to be landed. Let me know if it doesn't apply cleanly and I can try to rebase it.

Comment 25

3 months ago

Tried to land this and got:

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmp6fFAbD\npatching file xpcom/tests/gtest/TestGCPostBarriers.cpp\nHunk #1 FAILED at 83\n1 out of 1 hunks FAILED -- saving rejects to file xpcom/tests/gtest/TestGCPostBarriers.cpp.rej\npatching file js/src/fuzz-tests/tests.cpp\nHunk #1 FAILED at 22\n1 out of 1 hunks FAILED -- saving rejects to file js/src/fuzz-tests/tests.cpp.rej\nabort: patch failed to apply', '').

Flags: needinfo?(philip.chimento)
Keywords: checkin-needed
Assignee

Comment 26

3 months ago

I've updated the Phabricator revision, all seems to still work, here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb22c8274eb9fd228d43d4c985b98339f389fdf8

Flags: needinfo?(philip.chimento)
Assignee

Updated

3 months ago
Keywords: checkin-needed

Just tried to arc patch D11571, but rebase failed.

Flags: needinfo?(philip.chimento)
Assignee

Comment 28

3 months ago

It seems arc patch D11571 is somehow pulling in associated patches which have already been committed, so those are failing to rebase. I don't know how to get it to stop doing that.

This worked for me: arc patch --skip-dependencies --update D11571

Flags: needinfo?(philip.chimento)
Keywords: checkin-needed

Philip, I tried using your method and got hunks failed:

mozilla@ubuntu ~/mozilla-unified autoland(0) $ arc patch --skip-dependencies --u pdate D11571
Updating working copy...
Done.
Created and checked out bookmark arcpatch-D11571.

Patch Failed!
Exception
Command failed with error #255!
COMMAND
HGPLAIN=1 hg import --no-commit -

STDOUT
applying patch from stdin

STDERR
patching file js/src/fuzz-tests/tests.cpp
Hunk #1 FAILED at 22
1 out of 1 hunks FAILED -- saving rejects to file js/src/fuzz-tests/tests.cpp.re j
abort: patch failed to apply

(Run with --trace for a full exception trace.)

Flags: needinfo?(philip.chimento)

Aryx can you check out Comment 28?

Flags: needinfo?(aryx.bugmail)
Keywords: checkin-needed

Can you skip those committed patches and apply it on the top of mozilla-central, please? There are still conflicts when I do that with

arc patch --skip-dependencies --nobranch D11571

Flags: needinfo?(aryx.bugmail)
Assignee

Comment 32

3 months ago
Reviewers: tcampbell

Subscribers: jandem

Bug #: 1506324

Differential Revision: https://phabricator.services.mozilla.com/D11571
Assignee

Comment 33

3 months ago

I don't seem to get these conflicts...

In any case, I can't update the Phabricator revision anymore, so I've posted the patch here on Bugzilla instead.

Flags: needinfo?(philip.chimento)
Assignee

Updated

3 months ago
Attachment #9054775 - Flags: review+

Comment 34

3 months ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f44db83758c
Use DefaultGlobalClassOps in existing code. r=tcampbell

Comment 35

3 months ago
bugherder
Status: REOPENED → RESOLVED
Closed: 7 months ago3 months ago
Resolution: --- → FIXED
Target Milestone: mozilla65 → mozilla68
You need to log in before you can comment on or make changes to this bug.