453462 - TM: FCKeditor initialization breaks with JIT enabled (in 20080903 nightly)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Leon Sorokin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080903034741 Minefield/3.1b1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080903034741 Minefield/3.1b1pre

Firebug 1.2 output:
----------------------------
D is undefined
OutdentListItem()()fckedito..._gecko.js (line 52)
OutdentListItem()()fckedito..._gecko.js (line 52)
FCKToolbarFontFormatCombo()()fckedito..._gecko.js (line 91)
FCKToolbarFontFormatCombo()(Object FieldWidth=100 PanelWidth=150 PanelMaxHeight=150)fckedito..._gecko.js (line 91)
FCKToolbarStyleCombo()(td)fckedito..._gecko.js (line 90)
FCKToolbarBreak()(td#xToolbar.TB_ToolbarSet)fckedito..._gecko.js (line 97)
F()("Default")fckedito..._gecko.js (line 99)
LoadToolbar()fckedito...r=Default (line 232)
GetInstance()()KOJa7yj9...PRg%3D%3D (line 1)
GetInstance()(LoadToolbar())KOJa7yj9...PRg%3D%3D (line 1)
LoadToolbarSetup()fckedito...r=Default (line 220)
onload()()fckedito...r=Default (line 215)
[Break on this error] var FCKStyles=FCK.Styles={_Callbacks:{},...tyles[G]=true;A[Q.Name]=Q;};return A;}};

Reproducible: Always

Steps to Reproduce:
1. Initialize FCKeditor.
2.
3.
Actual Results:  
Javascript error

Expected Results:  
no errors.

Comment 1

[(not currently active) Ted Mielczarek]

Confirming, the editor fails to load with content JIT enabled, works with it disabled.

Comment 2

[(not currently active) Ted Mielczarek]

Attachment: reduced testcase

It might be possible to reduce this further, but I'm pretty sure removing any part of this causes it to pass. I'll post a testcase suitable for running in the shell once my build completes.

Comment 3

[Steffen Wilberg]

Is this the "editor on developer.mozilla.org doesn't display text with content JIT enabled" bug?

Comment 4

[Andreas Gal :gal]

This has been identified as an issue with taking a side exit in a scripted constructor, which requires special handling of the return value which the current side exit code doesn't do. mrbkap might whip up a patch for this today.

Comment 5

[Andreas Gal :gal]

mrbkap: along the same line we should also check whether we properly setup the call object when side-exiting from a heavyweight functions, or along a chain like lightweight -> heavyweight -> lightweight and then side-exit from the latter.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

I need to test this harder, but it appears to work.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Note: my patch doesn't take comment 5 into account. Someone should feel free to snatch that from me.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 337162 [details] [diff] [review]
Proposed fix

>-            if (!js_InvokeConstructor(cx, argc, vp))
>+            if (!js_InvokeConstructor(cx, argc, JS_FALSE, vp))

How is this safe? Who does the clamping? I'm not seeing it. The inline return case handles constructor return but that is disjoint from this case. Your patch adds no new clamping code, it only suppresses clamping from this call site.

> struct FrameInfo {
>     JSObject*       callee;     // callee function object
>     jsbytecode*     callpc;     // pc of JSOP_CALL in caller script
>+    uintN           flags;      // flags to set on the stack frame

Don't add another word to store -- do steal a bit from the spdist/argc pair for JSFRAME_CONSTRUCTING (which you can statically assert is 1, the LSb).

>+TraceRecorder::interpretedFunctionCall(jsval& fval, JSFunction* fun, uintN argc, uintN flags)

So flags is overlarge -- bool constructing is best.

/be

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #7)
> Note: my patch doesn't take comment 5 into account.

Please file a separate bug on the Call object for reconstructed heavyweight frames (yes, we'll need another bit -- still don't want to store flags in toto).

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #8)
> How is this safe? Who does the clamping? I'm not seeing it. The inline return
> case handles constructor return but that is disjoint from this case. Your patch
> adds no new clamping code, it only suppresses clamping from this call site.

The clamping is taken care of in the added code to the shared JSOP_RETURN/JSOP_STOP case lower in the patch (which is coincidentally the fix for the bug).

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #10)
> The clamping is taken care of in the added code to the shared
> JSOP_RETURN/JSOP_STOP case lower in the patch (which is coincidentally the fix
> for the bug).

I'm blind, this part's good. Can you make a new patch that avoids the extra rp store?

/be

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 337786 [details] [diff] [review]
Updated

>-            uint16  spdist;     // distance from fp->slots to fp->regs->sp at JSOP_CALL
>+            uint16  spdist : 15;// distance from fp->slots to fp->regs->sp at JSOP_CALL
>+            uint16  constructing : 1; // this frame JSFRAME_CONSTRUCTING
>             uint16  argc;       // actual argument count, may be < fun->nargs

Nits: prevailing style crunches (cuddles?) both sides : and give those comments some breathing room -- bump 'em over to the next 4-space stop.

>+    JS_ASSERT(!(fp->regs->sp - fp->slots & 0x8000));

This is ok, but we talked about stealing from argc instead. That's even less likely to be >= 32K.

>     FrameInfo fi = {
>         JSVAL_TO_OBJECT(fval),
>         fp->regs->pc,
>         typemap,
>-        { { fp->regs->sp - fp->slots, argc } }
>+        { { fp->regs->sp - fp->slots, constructing, argc } }

Bitfield initializers -- portable or not? Discuss.

r=me with sufficient fussing over above.

/be

Comment 16

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/3090e1560262

Comment 17

[Bob Clary [:bc] (inactive)]

also in js1_8_1/trace/trace-test.js

Comment 18

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2