Consider adding a JS_NewObject alternative API

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
JavaScript Engine
P1
major
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: mrbkap, Assigned: brendan)

Tracking

Trunk
mozilla1.9beta4
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
wanted1.9 +
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
JS_NewObject currently looks up the prototype object from the passed-in parent's scope. To do so, it does a property lookup and a get, neither of which are apparent when actually looking at the call. This can cause weird results when the parent is untrusted, making implementing wrappers just that much more difficult.

It might be worth it to add a new API, something like JS_NewNativeObject that avoids this step by either using some canonical Object.prototype or using a null prototype and leaving it up to the caller to decide if a prototype is actually necessary.
(Assignee)

Comment 1

11 years ago
I would feel safer with such an API and all the places that really want it changed to use it -- sure, we could audit, and perhaps we've patched enough JS_SetP{arent,rototype} calls in -- but those are code bloat and this is still an accident waiting to happen.

Putting on someone's radar (mine at least).

/be
Blocks: 380236
Version: unspecified → Trunk
(Assignee)

Comment 2

10 years ago
Easy way to win back Ts and gain on general XPConnected DOM perf.

/be
Assignee: general → brendan
Flags: wanted1.9+
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9beta3

Comment 3

10 years ago
Given comment 2 would like to see if we can get something in for Beta4
Flags: blocking1.9+
Priority: P1 → P2
Target Milestone: mozilla1.9beta3 → mozilla1.9beta4
(Assignee)

Comment 4

10 years ago
Want to address in-browser SunSpider regression (bug 415393) probably caused by bug 414452 with a fix to this bug, so making it block that one.

/be
Blocks: 414452
Severity: enhancement → major
Status: NEW → ASSIGNED

Updated

10 years ago
Priority: P2 → P1
(Assignee)

Comment 5

10 years ago
Created attachment 302246 [details] [diff] [review]
proposed fix

Others should weigh in. I chose a simple but imprecise name, JS_NewNativeObject, and avoided future-proofing. Thought about JS_NewObjectNoDefaultProto but it is just too long.

/be
Attachment #302246 - Flags: review?(mrbkap)
(Assignee)

Comment 6

10 years ago
Testing help, including any Ts/Txul testing, is welcome.

/be
How about JS_NewHostObject?  Native's overloaded in our world, though I suppose when called with a NULL clasp it's not creating a Host object in the ECMA sense.
(Reporter)

Comment 8

10 years ago
Comment on attachment 302246 [details] [diff] [review]
proposed fix

I don't have any strong feelings one way or the other about the name.

Want to also fix up the JS_NewObject in XPCCrossOriginWrapper.cpp while you're here?

Thanks.
Attachment #302246 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 9

10 years ago
Not "Host" object, that's just misleading in light of ECMA-262's usage. Arguably JS_NewObjectNoDefaultProto is best, but too long. Someone get inspired!

New patch with XOW case covered soon.

/be

Comment 10

10 years ago
JS_NewOrphanObject? (protoless, not parentless?  hmmm) JS_NewUnchainedObject?  Unfettered?
(Assignee)

Comment 11

10 years ago
It's not orphan or protoless, if you pass a non-null proto it's used just as in the JS_NewObject case. The only difference is the lack of default proto triggered by passing null.

/be
(Assignee)

Comment 12

10 years ago
Created attachment 303190 [details] [diff] [review]
proposed fix, better name and XPCCrossOriginWrapper.cpp changes
Attachment #302246 - Attachment is obsolete: true
Attachment #303190 - Flags: review?(mrbkap)
(Assignee)

Updated

10 years ago
Attachment #303190 - Attachment is patch: true
Attachment #303190 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 13

10 years ago
This will help Ts and probably Txul.

/be
(Reporter)

Updated

10 years ago
Attachment #303190 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 14

10 years ago
Created attachment 303203 [details] [diff] [review]
final patch for checkin

s/Exact/Given/g -- JS_NewObjectWithGivenProto. Good to be done with this, will keep an eye on Ts and Txul.

/be
Attachment #303190 - Attachment is obsolete: true
Attachment #303203 - Flags: review+
Attachment #303203 - Flags: approval1.9+
(Assignee)

Comment 15

10 years ago
Fix is in:

js/src/jsapi.c 3.407
js/src/jsapi.h 3.183
js/src/jsobj.c 3.429
js/src/jsobj.h 3.85
js/src/xpconnect/src/XPCCrossOriginWrapper.cpp 1.34
js/src/xpconnect/src/XPCNativeWrapper.cpp 1.73
js/src/xpconnect/src/XPCSafeJSObjectWrapper.cpp 1.28

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Depends on: 417819

Updated

10 years ago
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.