Closed Bug 229200 Opened 16 years ago Closed 16 years ago

Assertion botched adding onresize handler [@ js_UnlockScope ]

Categories

(Core :: JavaScript Engine, defect, P1, critical)

x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla1.7final

People

(Reporter: neil, Assigned: brendan)

Details

(Keywords: js1.5, regression)

Attachments

(2 files)

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
silver@warwickcompsoc.co.uk also reproduced this on his MingW build - and I also
see a crash (but no assertion) in a 3-week-old MingW build too.
OS: Linux → All
seems to wfm using FB 20031216 Win2k.
WFM using a 20031220 Linux CVS build.
Summary: Crash adding onresize handler → Crash adding onresize handler [@ js_UnlockScope ]
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?
Assignee: general → general
Component: JavaScript Engine → DOM: Level 0
QA Contact: pschwartau → ian
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"); }
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?
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
Assignee: general → brendan
Component: DOM: Level 0 → JavaScript Engine
Keywords: crashjs1.5
Priority: -- → P1
QA Contact: ian → pschwartau
Target Milestone: --- → mozilla1.7final
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
Summary: Crash adding onresize handler [@ js_UnlockScope ] → Assertion botched adding onresize handler [@ js_UnlockScope ]
Attached patch patch to trySplinter Review
I'm still not happy about it.  More thought required, but please test it if you
can and report results here.

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

WFM for the given testcase.
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
Attachment #145068 - Flags: review?(shaver)
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.
Attachment #145068 - Flags: review?(shaver) → review+
> 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
Flags: blocking1.7+
Attachment #145068 - Flags: approval1.7?
Comment on attachment 145068 [details] [diff] [review]
patch to try

a=chofmann for 1.7
Attachment #145068 - Flags: approval1.7? → approval1.7+
Fixed.

/be
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.