Closed Bug 1223 Opened 26 years ago Closed 25 years ago

Crash when handling js 'with' statement in NGLayout

Categories

(Core :: JavaScript Engine, defect, P1)

Product:
Core
Component:
JavaScript Engine
Platform:
x86
Other
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: angus, Assigned: shaver)

References

(
URL
)

Details

Attachments

(3 files)

Add JSPROP_ORIG_OBJ and use for __proto__
4.43 KB, patch
Details | Diff | Splinter Review
JSPROP_ORIG_OBJ mk 2 (brendan's nits and proper SPROP_OBJ use in js_LookupProperty)
5.74 KB, patch
Details | Diff | Splinter Review
Use OBJ_THIS_OBJECT in SPROP_[SG]ET
849 bytes, patch
Details | Diff | Splinter Review
Status: NEW → ASSIGNED
Okay, Vidur and I looked into this a bit.  Seems to be a problem with our
handling of 'with'.  We need to talk to some JS guys and figure out our
problem.
Summary: This page crashes NGLayout → Crash when handling js 'with' statement in NGLayout
Changing summary to more specific info.
Brendan said to bug you guys with this.  We seem to be having a problem where a
getProperty call inside a 'with' statement is passing us the with object, not
the object we're looking for.  He said this might be a regression.  Thoughts?
Brendan said to bug you guys with this.  We seem to be having a problem where a
getProperty call inside a 'with' statement is passing us the with object, not
the object we're looking for.  He said this might be a regression.  Thoughts?
Brendan said to bug you guys with this.  We seem to be having a problem where a
getProperty call inside a 'with' statement is passing us the with object, not
the object we're looking for.  He said this might be a regression.  Thoughts?
Before I look into this, you might want to try again with the latest JS-engine,
which just landed.  I think that norris fixed one or two 'with' bugs and it's
possible this is one of them.
Assignee: joki → fur
Status: ASSIGNED → NEW
Never mind what I just said about trying this with the latest version of the JS
engine.  I just built nglayout and the problem's still there.  I can see the
problem in the property code, but I'm not sure what the best way to fix it is.
I'm reassigning this to myself.
*** Bug 1353 has been marked as a duplicate of this bug. ***
*** Bug 1221 has been marked as a duplicate of this bug. ***
Priority: P2 → P1
Summary: Crash when handling js 'with' statement in NGLayout → ss:Crash when handling js 'with' statement in NGLayout
Putting on ss: radar.
*** Bug 1506 has been marked as a duplicate of this bug. ***
*** Bug 1576 has been marked as a duplicate of this bug. ***
This bug needs help from javascript team to fix this.  shaver?  mlm?  Need help
ASAP please.
Summary: ss:Crash when handling js 'with' statement in NGLayout → Crash when handling js 'with' statement in NGLayout
This bug did not make ss: train for this release.  Removing ss: freom summary.
Status: NEW → ASSIGNED
Scott, when can you get this done? I've got yet another bug that's related to
this.
Sorry for letting this sit on the backburner for so long.  I'm checking to see
if mccabe can handle it - if not, I'll get to it at the beginning of next week.
Assignee: fur → mccabe
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
I poked around a www.loa.com with a recent SeaMonkey and didn't run into any
trouble.  Fur, what's the problem in the property code that you mention?  Do you
have a testcase I can use?
This was a bug that I had asked your advice on a long time ago - someone made
a deliberate change to the per-property getter code, but it was not clear
why/how the change was introduced or what else might break by fixing it.

As I recall, the wrong object gets passed to a per-property-getter when a
property was accessed inside a with statement.  The With object gets passed in
instead of the With's prototype object.  In this case, the getter expected a
host object, so it assert-botched in JS_GET_PRIVATE().

You can try some of the other bugs which DUPLICATE the problem, e.g. bug 1221
uses http://www.microsoft.com/presspass/trial/default.htm
Setting all current Open Critical and Major to M3
This definitely still exists.  Mike, Vidur and I decided you can't move in here
till you fix this.  If you still can reproduce it, just come to one of our
machines.
I just tried a release build (Mar 2nd) on Windows NT and could not get any of
the cited URLs to crash.   Maybe this bug only affects the debug builds.
per leger, assigning QA contacts to all open bugs without QA contacts according
to list at http://bugzilla.mozilla.org/describecomponents.cgi?product=Browser
Related bug 1322 - http://developer.netscape.com/viewsource/index_frame.html,
page which used to crash but now does not crash but will not layout, think it is
same issue?
QA Contact: 4015 → 4590
Christine, is this yours?
QA Contact: 4590 → 4015
apparently, it's only a problem with host objects (like window and document),
and not reproducible from the engine alone.  i'm not maintaining any html
regression stuff so it'd be better if you guys held on to this one.  if that's
okay with you.

i think this reproduces the problem:
<script>
with ( document ) { open(); write ( lastModified ); close(); }
</script>

or you could just enter the url
javascript:with ( document ) { open(); write ( lastModified ); close(); }

(just in case they change content of the URL above).
Target Milestone: M3 → M4
sounds like a problem that has been around awhile.  We ought to try and
nail it form M4.
*** Bug 4160 has been marked as a duplicate of this bug. ***
Setting this bug to m5.  Yes, I hope to get to it rsn.
whoops, actually changing the setting...
Target Milestone: M4 → M5
Hi guys, I reported bug 4160 which seems to be the same error.
You can see this crash in the current CVS build on Wed Apr 14,
Solaris 5.6 Sparc with egcs. You can see this crash by going
to the URL http://developer.netscape.com/tech/java/index.html.

./viewer
Warning: MOZILLA_FIVE_HOME not set.
nsComponentManager: Using components dir:
/project/neon/users/mo/mozilla/mozilla/solaris/dist/bin/components
Going to create the event queue
GFX: dpi=96 t2p=0.0666667 p2t=15 depth=8
Using '/project/neon/users/mo/mozilla/mozilla/solaris/dist/bin' as the resource:
base
Got thew event queue from the service
Calling gdk_input with event queue
Reading file...
Reading file...Done
Note: frameverifytree is disabled
Note: verifyreflow is disabled
Assertion failure: OBJ_GET_CLASS(cx, obj)->flags & JSCLASS_HAS_PRIVATE, at
../../../js/src/jsapi.c:1081
Abort (core dumped)
Fur: what change was it that caused this behaviour?  I could spend some time
this weekend looking at it (fiancee out of town!), since I think mccabe is too
doomed for M5 already.
I don't remember the details, except that it only shows up if per-property
getters are used.  The code around the crash was heavily modified from
a previous version (that did not have this bug), but I couldn't figure
out why the changes were made.
Component: DOM Level 1 → Javascript Engine
OK, here's my fix:

jsscope.h:
 /*
  * These macros are designed to decouple getter and setter from sprop, by
  * passing obj2 (in whose scope sprop lives, and in whose scope getter and
  * setter might be stored apart from sprop -- say in scope->opTable[i] for
  * a compressed getter or setter index i that is stored in sprop).
  */
-#define SPROP_GET(cx,sprop,obj,obj2,vp) ((sprop)->getter(cx,obj,sprop->id,vp))
-#define SPROP_SET(cx,sprop,obj,obj2,vp) ((sprop)->setter(cx,obj,sprop->id,vp))
+#define SPROP_GET(cx,sprop,obj,obj2,vp) ((sprop)->getter(cx,obj2,sprop->id,vp))
+#define SPROP_SET(cx,sprop,obj,obj2,vp) ((sprop)->setter(cx,obj2,sprop->id,vp))

This certainly seems like the right thing: we want to be calling the getting
with the object on which the property was found, not the one on which we started
the search.

REGRESSION: This breaks __proto__.  All other property accesses (including
__parent__) seem to work correctly, though.  js_LookupProperty doesn't find
__proto__ in the inital obj, so it goes and finds it in the prototype, and then
because of the fix it actually uses the prototype for the getter call.  The
prototype's __proto__ is null, so that's what it returns.  I _think_ the right
fix is to teach js_LookupProperty about the __proto__ magic, although maybe we
should add a flag to JSScopeProperty that tells us to use the original object
instead.  I'll work up a diff to that effect and attach it here, if it helps.
The attached diff makes __proto__ do the right thing, and doesn't seem to trip
any tests (though I'm running them manually, so I might have missed some).  Can
someone (fur? mccabe? brendan? norris?) comment?
Hack, cough, groan.  Fur, hurry up with JS2 willya.

This fix looks ok, it uses a spare flag rather than one (or three) atom compares
for the ___foo__ properties.  A couple of comments:

1.  Doesn't __parent__ need JSPROP_ORIG_OBJ too?

2.  Use (uint8) attrs rather than attrs & 0xff when storing in sprop->attrs, and
make it easier on the optimizer.

3.  Use js_proto_str not "__proto__" in js_InitObjectClass.

Thanks for fixing this, shaver.

/be
OK, sure.

christine reports that my patch also breaks "Boolean.prototype.__proto__" and
"Function.__proto__", which is true.  __proto__ continues to work for objects
created in JS, but seems to fail for those created by js_NewObject.  I'm not
quite sure why, but I'll take a longer look today.
OK, the new diff should be much better.  ``function f() { }; f.__proto__''
works, as do ``Boolean.prototype__proto__'' and the like.  (I think that JS2
should pass both obj and obj2 to getters, for the record, so that the getter
implementor can choose whatever behaviour it wants.)
Not done yet (I broke ``arguments'').  If you want to stick this in M5, I'll
take the bug assigned to me and commit to fixing arguments ASAP.  I think
arguments is a lesser breakage than ``with (document)'', so this might be a
forward-progress move, but it's the JS group's call.
If Tinderbox clears up by tomorrow, I plan to drop the SpiderMonkey140_BRANCH
onto mozilla then.  We've run the full test suite on most platforms with the so
it's a known quantity and we have high confidence in it.  I have a slight
preference for us waiting until just after M5 to put in this "with" fix.
Shaver and I were just talking about this (again), and we have a slightly
cleaner solution that will allow us to flag magic properties in their
JSPropertySpec initializers.  We need to do that for arguments as well as
__proto__ and __parent__, probably for a few more magic ones (in jsfun.c at
least).  I'd like to see the fix diffs before deciding yea or nay for M5, but it
is up to you guys: say "wait" if you must.

/be
I'm now seeing weird property-cache interactions with __caller__ (the sprop that
comes out the second time is _not_ what goes in), so I agree that we should wait
until after M5.

Continuing to plug away...
More thoughts: There are a lot of functions out there that depend on the
current, arguably broken behaviour.  Many of those are in the engine itself, and
I can fix those, but I'm afraid of what will happen with all the other host
objects out there in other people's code.

I want to look at this some more, because ``with (new String("foo")) { print
(length); }'' works as I would expect, so there's something subtle here.
Assignee: mccabe → shaver
Status: ASSIGNED → NEW
Target Milestone: M5 → M6
This is not going to be fixed for M5, embarrassingly enough.  Also, I think it's
my bug now.
Status: NEW → ASSIGNED
Hi all. I noticed that there was a change that might fix this bug. I did
a CVD build today (Tue May 11) and went to this url
http://developer.netscape.com/tech/java/index.html and I still got the
crash.

FindBookmarkShortcut: in='http://developer.netscape.com/tech/java/index.html'
out=''
Assertion failure: OBJ_GET_CLASS(cx, obj)->flags & JSCLASS_HAS_PRIVATE, at
../../../js/src/jsapi.c:1083
Abort (core dumped)

I hope that helps
Third time's a charm.  Newsgroup and email discussions have led me to believe
that this fix (attached above) is the right mix of ``better behaviour'' and
``unintrusive surgery''.  I haven't run the JS test suite yet, but I will do so
tomorrow AM and then likely commit the change on the branch and tip.

And then I will drink in celebration, for 1223 will be dead, dead, dead.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
No apparent regressions in the JS test suite, so I've checked it in.  We no
longer crash on http://www.loa.com, so I'm marking it fixed.
Status: RESOLVED → VERIFIED
Verified on 1999-05-18-09 build on Windows NT.
bulk reassigning all bugs from shaver's @netscape account to his @mozill acount
Status: RESOLVED → VERIFIED
Marking Verified again, as the status changed to Resolved by the bulk reassign.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: