1506324 - Add JS::DefaultGlobalClassOps to JSAPI

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Philip Chimento [:ptomato]]

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.

Comment 1

[Ted Campbell [:tcampbell]]

I wonder if DefaultGlobalClassOps is a better connotation than "simple".

Comment 2

[Philip Chimento [:ptomato]]

Agreed. Also I wasn't sure between JS::DefaultGlobalClassOps and JSDefaultGlobalClassOps so I went with the former.

Comment 3

[Ted Campbell [:tcampbell]]

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.

Comment 4

[Philip Chimento [:ptomato]]

Attachment: Bug 1506324 - Add JS::DefaultGlobalClassOps to JSAPI. r=tcampbell

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

Depends on D11569

Comment 5

[Philip Chimento [:ptomato]]

Attachment: Bug 1506324 - Use DefaultGlobalClassOps in existing code. r=tcampbell

Depends on D11570

Comment 6

[Jason Orendorff [:jorendorff]]

ptomato and I discussed this on IRC a few days ago. I'd like to see this land.

Comment 7

[Philip Chimento [:ptomato]]

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.

Comment 8

[Jason Orendorff [:jorendorff]]

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.

Comment 9

[Philip Chimento [:ptomato]]

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.

Comment 10

[Philip Chimento [:ptomato]]

Thanks for the review! So, the procedure now is that I attach the revisions from Phabricator to this bug again, and add checkin-needed?

Comment 11

[Philip Chimento [:ptomato]]

Attachment: Add JS::DefaultGlobalClassOps to JSAPI

Reviewers: jorendorff

Subscribers: jandem

Bug #: 1506324

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

Comment 12

[Philip Chimento [:ptomato]]

Attachment: Use DefaultGlobalClassOps in existing code

Summary: Depends on D11570

Reviewers: tcampbell

Reviewed By: tcampbell

Subscribers: jandem

Bug #: 1506324

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7be95c95a3a7
Add JS::DefaultGlobalClassOps to JSAPI. r=jorendorff

Comment 14

[Cosmin Sabou [:CosminS]]

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

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7be95c95a3a7

Comment 16

[Jan de Mooij [:jandem]]

(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 :)

Comment 17

[Philip Chimento [:ptomato]]

Attachment: Use DefaultGlobalClassOps in existing code

Summary: Depends on D11570

Reviewers: tcampbell, jorendorff

Reviewed By: tcampbell, jorendorff

Subscribers: jandem

Bug #: 1506324

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Andrei Ciure[:aciure]]

Backed out 2 changesets (bug 1506324) for bustages error: redefinition of 'DefaultGlobalClassOps'

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&group_state=expanded&selectedJob=212705354&revision=412920e602faa64d31b4a07a8d9d068413f04bc0

failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&group_state=expanded&fromchange=d32110a492f5eece3697fd039d53dba247f9f202&selectedJob=212705354&searchStr=linux%2Cx64%2Copt%2Cbuild-linux64%2Fopt%2C%28b%29

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/1eeecfde9fe1d3258fc86782275b056a38800d73

Comment 20

[Philip Chimento [:ptomato]]

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.

Comment 21

[Ted Campbell [:tcampbell]]

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.

Comment 22

[Philip Chimento [:ptomato]]

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

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

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?

Comment 24

[Philip Chimento [:ptomato]]

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

[Razvan Maries]

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', '').

Comment 26

[Philip Chimento [:ptomato]]

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

Comment 27

[Bogdan Tara[:bogdan_tara | bogdant]]

Just tried to arc patch D11571, but rebase failed.

Comment 28

[Philip Chimento [:ptomato]]

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

Comment 29

[Andreea Pavel [:apavel]]

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.)

Comment 30

[Andreea Pavel [:apavel]]

Aryx can you check out Comment 28?

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 32

[Philip Chimento [:ptomato]]

Attachment: Use DefaultGlobalClassOps in existing code

Reviewers: tcampbell

Subscribers: jandem

Bug #: 1506324

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

Comment 33

[Philip Chimento [:ptomato]]

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.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f44db83758c
Use DefaultGlobalClassOps in existing code. r=tcampbell

Comment 35

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/1f44db83758c