Closed Bug 570662 Opened 11 years ago Closed 11 years ago

TM: eagerly call thisObject hook and cache result in global objects

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: gal, Assigned: jorendorff)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 2 obsolete files)

This patch does less than we could do, but I ran into a few complications. Also, its not entirely clear this is safe. If this works, we can rip out JSFRAME_CALCULATED_THIS and remove fp->thisv, but that patch is pretty involved, so lets do this one by one.
Attached patch patch (obsolete) — Splinter Review
Assignee: general → gal
Things that worry me: thisObject hook runs when the global object is made, not when accessing .this, so stack and scope chain might look different. Also, invalidation? Or is this always safe? With our cross compartment wrappers this should be safe for sure. Right now not so sure.
Attachment #449804 - Flags: review?(mrbkap)
Note the code around this comment:

/* Now that the frame has been pushed, we can call the thisObject hook. */

Thats basically exactly what worries me.
Blake noticed that if you call thisObject from JS_SetGlobalObject, it will call JS_GetScopeChain and get back a wrong answer (likely NULL). So we wait until the first time you need it.
Assignee: gal → jorendorff
Attachment #449804 - Attachment is obsolete: true
Attachment #454088 - Flags: review?(mrbkap)
Attachment #449804 - Flags: review?(mrbkap)
Attached patch v2Splinter Review
Attachment #454088 - Attachment is obsolete: true
Attachment #454096 - Flags: review?(mrbkap)
Attachment #454088 - Flags: review?(mrbkap)
Comment on attachment 454096 [details] [diff] [review]
v2

mrbkap says it's ok
Attachment #454096 - Flags: review?(mrbkap) → review+
Whiteboard: fixed-in-tracemonkey
Also requires this follow-up fix: http://hg.mozilla.org/tracemonkey/rev/f454bf6a0693
You need to log in before you can comment on or make changes to this bug.