229200 - Assertion botched adding onresize handler [@ js_UnlockScope ]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[[e-mail]]

Steps to reproduce problem:
1. Open JS console
2. Type
top.onresize = function() { alert("resize"); }
3. Click Evaluate

Assertion failure: scope->ownercx == NULL, at jslock.c:1084

Comment 1

[[e-mail]]

[e-mail] also reproduced this on his MingW build - and I also
see a crash (but no assertion) in a 3-week-old MingW build too.

Comment 2

[Olivier Cahagne]

seems to wfm using FB 20031216 Win2k.

Comment 3

[Oliver Klee]

WFM using a 20031220 Linux CVS build.

Comment 4

[[e-mail]]

Attachment: Extract of stack backtrace

Comment 5

[Alex Fritze]

I'm seeing the same problem with onpaint.
I don't think this is a JavaScript engine bug as such. It looks like
nsEventReceiverSH::AddProperty is doing something naughty. Changing component to
DOM:Level 0.
Maybe this is regression caused by bug#147811?

Comment 6

[Alex Fritze]

A couple of observations:

1. This bug seems to occur only when the function to be attached is from a
different JS context. Setting a listener from a html file works fine - see
testcase at http://www.croczilla.com/~alex/onresize.html (the corresponding
testcase for onpaint doesn't work, but that's a different issue, reported in
bug#239074).

2. There is a workaround to prevent the assertion/crash - just set the handler
to some string before attaching the 'real' handler:
 top.onresize = "foo";
 top.onresize = function() { alert("resize"); }

Comment 7

[Johnny Stenback (:jst)]

Brendan, this is the same thing that we ran into with that Java applet
testcase... Did you have some ideas on what to do about this?

Comment 8

[Brendan Eich [:brendan]]

An assertion botch is not a crash bug in release builds.  It looks like from the
WFM comments that the release builds have no problem, as expected.  Rearranging
keywords and taking the bug.

/be

Comment 9

[Brendan Eich [:brendan]]

Anyone who sees a crash in release builds, please tell me more about it -- best
would be to get the line number of the crash, if possible.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: patch to try

I'm still not happy about it.  More thought required, but please test it if you
can and report results here.

/be

Comment 11

[[e-mail]]

Comment on attachment 145068 [details] [diff] [review]
patch to try

WFM for the given testcase.

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 145068 [details] [diff] [review]
patch to try

Shaver, this is all about avoiding assertions in the Mozilla client world. 
Maybe I should #ifdef MOZILLA_CLIENT the new code, #else'ing to the assertion
line that's removed by this patch?

/be

Comment 13

[Mike Shaver (:shaver emeritus)]

Comment on attachment 145068 [details] [diff] [review]
patch to try

This asymmetric thread model seems like something that could appear to more
embeddings than just Mozilla's, so I think I'd prefer to add a #define for
something like NESTED_CONTEXTS_PER_THREAD and set that #if MOZILLA_CLIENT, but
now I'm just being picky.  The patch looks good, to me.

Comment 14

[Brendan Eich [:brendan]]

> This asymmetric thread model seems like something that could appear to more
> embeddings than just Mozilla's, so I think I'd prefer to add a #define for
> something like NESTED_CONTEXTS_PER_THREAD and set that #if MOZILLA_CLIENT, but
> now I'm just being picky.

It's not just the N:1 context:thread relation in Mozilla, it's the utter lack of
requests being begun and ended, suspended and resumed.  I'd hope other
embeddings use requests scrupulously, even if they have N>1.

Still thinking how to tighten this up, but I'd like to get the assertion removal
in for 1.7.

/be

Comment 15

[chris hofmann]

Comment on attachment 145068 [details] [diff] [review]
patch to try

a=chofmann for 1.7

Comment 16

[Brendan Eich [:brendan]]

Fixed.

/be