pushobj is not GC-atomic when debugger is installed.

RESOLVED FIXED in mozilla1.9alpha1

Status

()

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

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({fixed1.8.0.10, fixed1.8.1.2})

Trunk
mozilla1.9alpha1
fixed1.8.0.10, fixed1.8.1.2
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate])

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
Currently SpiderMonkey implements function call operator using a helper pushobj bytecode that pushes the obj register to the stack. The register is populated during the previous JSOP_GETPRROP, JS_NAME etc. operation. The problem is that the register does not belong to a root set so with the debugger installed a call like "1()" can be a GC hazard. This will happen when the debugger is called between "name" and "pushobj" bytecode and when the debugger calls JS_GC or JS_MaybeGC() at that point.

Updated

11 years ago
Assignee: general → brendan
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha

Comment 1

11 years ago
Should registers always be considered as GC roots?
(In reply to comment #1)
> Should registers always be considered as GC roots?

No, that's conservative GC (indeed we can't know whether obj is in a register or homed to a stack word, so conservative scanning of thread stacks would be needed, not just of thread register sets).

The fix is to compress bytecode so that JSOP_GETMETHOD or newer forms always push two jsvals: callee, thisp.

/be
Status: NEW → ASSIGNED
(Assignee)

Comment 3

11 years ago
(In reply to comment #2)
> The fix is to compress bytecode so that JSOP_GETMETHOD or newer forms always
> push two jsvals: callee, thisp.

Or alternatively to avoid proliferation of bytecodes [call] itself can be turned into a prefix opcode that sets a flag. Then [name] etc. would need to check for it and jump to to a common call label when they are done. This would removes the need for [pushobj]. 

Yet another alternative is to have [call_name], [call_local], [call_prop] etc. (which is similar to the current [foreach_name])] that calculates this and function and again jump to the common call code.
(In reply to comment #3)
> Yet another alternative is to have [call_name], [call_local], [call_prop] etc.
> (which is similar to the current [foreach_name])] that calculates this and
> function and again jump to the common call code.

Have to take care not to violate guaranteed order of evaluation, which is left to right, callee before arguments.

/be
(Assignee)

Comment 5

11 years ago
(In reply to comment #4)
> Have to take care not to violate guaranteed order of evaluation, which is left
> to right, callee before arguments.

Right, [call_prop] does not work as arguments can mutate the value of the property. 

Then a propere fix is either indeed to add [name_and_this], [var_and_this] etc. that push to the stack both this and function (which is how Rhino implements it), or turn [pushobj] into a prefix bytecode and make all [name], [var] check/reset a global flag set by [pushobj]. 

On the other hand a quick and dirty fix is not to call the debugger for pushobj.  This is good for branches.
Whiteboard: [sg:moderate]
Created attachment 248339 [details] [diff] [review]
patch to record bytecode successor counts

This produces a dot file, usable by graphviz, dotty, etc.  Sample output for perfect.js next.

/be
Created attachment 248340 [details]
bytecode graph for perfect.js

Lots of room for optimization here.  A browser graph for startup and load of my gmail folder is coming next (takes a while to render).

/be
Created attachment 248343 [details] [diff] [review]
filter edges < 1% of grand total bytecode count
Attachment #248339 - Attachment is obsolete: true
Created attachment 248344 [details]
filtered gmail load and first message view bytecode graph
* Assignment (set*) bytecodes should pop, which means assignment expressions whose results are used (e.g. for other assignments, or in actual arguments) must dup their RHS before assigning and popping.  This is bug 312354 in disguise.

* Lots of unqualified names being called: getmethod and name => pushobj.  Covered by this bug.

* lt, etc. => ifeq, a bug to be filed shortly.

* this => getprop, TBF.

/be
Tracked by cover bug 363530, which blocks bytecode-optimation meta bug 363529.

/be
(Assignee)

Comment 12

11 years ago
Created attachment 248752 [details] [diff] [review]
Minimal fix v1

The patch makes sure that interruptHandler is never called for pushobj. In the threaded case for this the patch records L_JSOP_PUSHOBJ in interruptJumpTable so the handler never gets the control for the opcode. In the switch case the code checks directly for pushobj when calling interruptHandler. 

This is a minimal fix to make easy branch patching.
Assignee: brendan → igor.bukanov
Attachment #248752 - Flags: review?(brendan)
Comment on attachment 248752 [details] [diff] [review]
Minimal fix v1

Thanks, good for the branches.

I'm planning to hack on bytecode soon, did you want to hack the long-term patch for this bug?

/be
Attachment #248752 - Flags: review?(brendan)
Attachment #248752 - Flags: review+
Attachment #248752 - Flags: approval1.8.1.2?
Attachment #248752 - Flags: approval1.8.0.10?
(Assignee)

Comment 14

11 years ago
(In reply to comment #13)
> (From update of attachment 248752 [details] [diff] [review] [edit])
> Thanks, good for the branches.
> 
> I'm planning to hack on bytecode soon, did you want to hack the long-term patch
> for this bug?

Yep, I can try to introduce the extra bytecodes like [name_and_this], [prop_and_this] etc. But the names are what I put 3 years ago to Rhino and, I guess, are too long for bytecode nicks. Any better idea?
We could build on JSOP_GETMETHOD.

NAME    : GETNAMEDMETHOD
GETPROP : GETMETHOD
GETELEM : GETINDEXEDMETHOD
CALL    : SETCALL

I think this works, but it's not parallel, and the MM in GETELEMMETHOD is ugly.

How about renaming GETMETHOD?

NAME    : NAMECALL
GETPROP : PROPCALL
GETELEM : ELEMCALL

Confusion: these are not call ops, but "eval and push [callee, this]" ops.  But it's not really that confusing, IMHO.  If you want, you could add "EE" at the end for "CALLEE".

/be
(Assignee)

Comment 16

11 years ago
(In reply to comment #15)
> We could build on JSOP_GETMETHOD.

Currently JSOP_GETMETHOD is used to implement function::x not followed by the method call:

function f(foo)
{
	return foo.function::bar;
}

dis(f);

00000:  getarg 0
00003:  getmethod "bar"
00006:  return
00007:  stop
 
With the new semantics [propcall] would push both function and this object so function:: would have to be implemented via [propcall]/[pop] pair. Then "call" in [propcall] is confusing. So indeed [propcallee] would be better.

But what about then using "this" as a suffix, like in:

PROPTHIS
NAMETHIS
ELEMTHIS

Or perhaps "obj":
PROPOBJ
NAMEOBJ
ELEMOBJ

?
 
(Assignee)

Comment 17

11 years ago
Ignore the prev comment. I forgot about the suggestion to rename [call] to [set_call]. Then

PROPCALL
NAMECALL
ELEMCALL

are nice and simple.
Sorry, I didn't mean to propose renaming JSOP_CALL to JSOP_SETCALL -- the latter already exists for o.item(i) = j (MS JScript compatibility, native o.item only).

NAMETHIS PROPTHIS ELEMTHIS are better, if you ask me -- why did you prefer CALL in the name?

/be
(Assignee)

Comment 19

11 years ago
(In reply to comment #18)
> 
> NAMETHIS PROPTHIS ELEMTHIS are better, if you ask me -- why did you prefer CALL
> in the name?

Because it reminds about a call context. NAMETHIS also reminds about it, but not that strongly. So as the last attempt and following FORNAME, FORPROP etc. tradition, what about:

FCNAME
FCPROP
FCELEM
FCLOCAL
FCARG
FCVAR

where FC stands for Function call Context and means to push both function and this.

?

FC -- too concise, crypic without more precedent; context not enough to decrypt. I'd go back to CALL{NAME,PROP,ELEM} if you want CALL more than THIS.

/be
(Assignee)

Comment 21

11 years ago
I committed the patch from comment 12 to the trunk:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.309; previous revision: 3.308
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Comment on attachment 248752 [details] [diff] [review]
Minimal fix v1

approved for 1.8/1.8.0 branches, a=dveditz for drivers
Attachment #248752 - Flags: approval1.8.1.2?
Attachment #248752 - Flags: approval1.8.1.2+
Attachment #248752 - Flags: approval1.8.0.10?
Attachment #248752 - Flags: approval1.8.0.10+
(Assignee)

Comment 23

11 years ago
I committed the patch from comment 12 to MOZILLA_1_8_BRANCH:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.77; previous revision: 3.181.2.76
done
 
Keywords: fixed1.8.1.2
(Assignee)

Comment 24

11 years ago
Created attachment 249054 [details] [diff] [review]
1.8.0 version of Minimal fix v1 

1.8.0 branch does not use indirect threading. That required not-so-trivial efforts to backport the patch there. hence I ask for another approval/review.
Attachment #249054 - Flags: review?(brendan)
Attachment #249054 - Flags: approval1.8.0.10?
Comment on attachment 249054 [details] [diff] [review]
1.8.0 version of Minimal fix v1 

Sure, only hurts perf when interruptHandler is set.

/be
Attachment #249054 - Flags: review?(brendan) → review+
(Assignee)

Comment 26

11 years ago
Comment on attachment 248752 [details] [diff] [review]
Minimal fix v1

For 1.8.0 see the next attachment.
Attachment #248752 - Flags: approval1.8.0.10+

Comment 27

11 years ago
Comment on attachment 249054 [details] [diff] [review]
1.8.0 version of Minimal fix v1 

Approved for the 1.8.0 branch, a=jay for drivers.
Attachment #249054 - Flags: approval1.8.0.10? → approval1.8.0.10+
(Assignee)

Comment 28

11 years ago
I committed the patch from comment 24 to MOZILLA_1_8_0_BRANCH:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.17.2.24; previous revision: 3.181.2.17.2.23
done
Keywords: fixed1.8.0.10

Updated

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