Closed Bug 1506324 Opened 6 years ago Closed 6 years ago

Add JS::DefaultGlobalClassOps to JSAPI

Categories

(Core :: JavaScript Engine, enhancement, P2)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: ptomato, Assigned: ptomato)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

Bug 1506324 - Use DefaultGlobalClassOps in existing code. r=tcampbell
(deleted), text/x-phabricator-request
Details
Add JS::DefaultGlobalClassOps to JSAPI
(deleted), patch
ptomato
: review+
Details | Diff | Splinter Review
Use DefaultGlobalClassOps in existing code
(deleted), patch
ptomato
: review+
Details | Diff | Splinter Review
Use DefaultGlobalClassOps in existing code
(deleted), patch
ptomato
: review+
Details | Diff | Splinter Review
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: nobody → philip.chimento
Status: NEW → ASSIGNED
I wonder if DefaultGlobalClassOps is a better connotation than "simple".
Summary: Add JSSimpleGlobalClassOps to JSAPI → Add JS::DefaultGlobalClassOps to JSAPI
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
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
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.
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.
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)
Reviewers: jorendorff Subscribers: jandem Bug #: 1506324 Differential Revision: https://phabricator.services.mozilla.com/D11570
Attachment #9024222 - Attachment is obsolete: true
Attached patch Use DefaultGlobalClassOps in existing code (obsolete) (deleted) — — Splinter Review
Summary: Depends on D11570 Reviewers: tcampbell Reviewed By: tcampbell Subscribers: jandem Bug #: 1506324 Differential Revision: https://phabricator.services.mozilla.com/D11571
Attachment #9024223 - Attachment is obsolete: true
Attachment #9025832 - Flags: review+
Attachment #9025833 - Flags: review+
Keywords: checkin-needed
Attachment #9024222 - Attachment is obsolete: false
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)
https://hg.mozilla.org/mozilla-central/rev/7be95c95a3a7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65: affected → fixed
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 → ---
Summary: Depends on D11570 Reviewers: tcampbell, jorendorff Reviewed By: tcampbell, jorendorff Subscribers: jandem Bug #: 1506324 Differential Revision: https://phabricator.services.mozilla.com/D11571
Attachment #9024222 - Attachment is obsolete: true
Attachment #9025833 - Attachment is obsolete: true
Flags: needinfo?(philip.chimento)
Attachment #9026131 - Flags: review+
Attachment #9024223 - Attachment is obsolete: false
Keywords: checkin-needed
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
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.
status-firefox65: affected → wontfix
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)
Flags: needinfo?(philip.chimento)
Keywords: checkin-needed

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.

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

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)
Keywords: checkin-needed

Just tried to arc patch D11571, but rebase failed.

Flags: needinfo?(philip.chimento)

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)
Reviewers: tcampbell Subscribers: jandem Bug #: 1506324 Differential Revision: https://phabricator.services.mozilla.com/D11571

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)
Attachment #9054775 - Flags: review+
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/1f44db83758c Use DefaultGlobalClassOps in existing code. r=tcampbell

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

Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: mozilla65 → mozilla68
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: