Crash when handling js 'with' statement in NGLayout

VERIFIED FIXED in M6

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
20 years ago
17 years ago

People

(Reporter: Angus Davis, Assigned: shaver)

Tracking

Trunk
x86
Other
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

Updated

20 years ago
Status: NEW → ASSIGNED

Description

20 years ago
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.

Updated

20 years ago
Summary: This page crashes NGLayout → Crash when handling js 'with' statement in NGLayout

Comment 1

20 years ago
Changing summary to more specific info.

Comment 2

20 years ago
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?

Comment 3

20 years ago
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?

Comment 4

20 years ago
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?

Comment 5

20 years ago
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.

Updated

20 years ago
Assignee: joki → fur
Status: ASSIGNED → NEW

Comment 6

20 years ago
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.

Comment 7

20 years ago
*** Bug 1353 has been marked as a duplicate of this bug. ***

Comment 8

20 years ago
*** Bug 1221 has been marked as a duplicate of this bug. ***

Updated

20 years ago
Priority: P2 → P1
Summary: Crash when handling js 'with' statement in NGLayout → ss:Crash when handling js 'with' statement in NGLayout

Comment 9

20 years ago
Putting on ss: radar.

Comment 10

20 years ago
*** Bug 1506 has been marked as a duplicate of this bug. ***

Comment 11

20 years ago
*** Bug 1576 has been marked as a duplicate of this bug. ***

Comment 12

20 years ago
This bug needs help from javascript team to fix this.  shaver?  mlm?  Need help
ASAP please.

Updated

20 years ago
Summary: ss:Crash when handling js 'with' statement in NGLayout → Crash when handling js 'with' statement in NGLayout

Comment 13

20 years ago
This bug did not make ss: train for this release.  Removing ss: freom summary.

Updated

20 years ago
Status: NEW → ASSIGNED

Comment 14

20 years ago
Scott, when can you get this done? I've got yet another bug that's related to
this.

Comment 15

20 years ago
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.

Updated

20 years ago
Assignee: fur → mccabe
Status: ASSIGNED → NEW

Updated

20 years ago
Status: NEW → ASSIGNED

Comment 16

20 years ago
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?

Comment 17

20 years ago
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

Comment 18

20 years ago
Setting all current Open Critical and Major to M3

Comment 19

20 years ago
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.

Comment 20

19 years ago
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.

Comment 21

19 years ago
per leger, assigning QA contacts to all open bugs without QA contacts according
to list at http://bugzilla.mozilla.org/describecomponents.cgi?product=Browser

Comment 22

19 years ago
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?

Updated

19 years ago
QA Contact: 4015 → 4590

Comment 23

19 years ago
Christine, is this yours?

Updated

19 years ago
QA Contact: 4590 → 4015

Comment 24

19 years ago
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).

Updated

19 years ago
Target Milestone: M3 → M4

Comment 25

19 years ago
sounds like a problem that has been around awhile.  We ought to try and
nail it form M4.

Comment 26

19 years ago
*** Bug 4160 has been marked as a duplicate of this bug. ***

Comment 27

19 years ago
Setting this bug to m5.  Yes, I hope to get to it rsn.

Comment 28

19 years ago
whoops, actually changing the setting...
Target Milestone: M4 → M5

Comment 29

19 years ago
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.

Comment 31

19 years ago
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.

Updated

19 years ago
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.
Created attachment 57 [details] [diff] [review]
Add JSPROP_ORIG_OBJ and use for __proto__
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?

Comment 35

19 years ago
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.
Created attachment 63 [details] [diff] [review]
JSPROP_ORIG_OBJ mk 2 (brendan's nits and proper SPROP_OBJ use in js_LookupProperty)
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.

Comment 40

19 years ago
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.

Comment 41

19 years ago
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.

Updated

19 years ago
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.

Updated

19 years ago
Status: NEW → ASSIGNED

Comment 45

19 years ago
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
Created attachment 154 [details] [diff] [review]
Use OBJ_THIS_OBJECT in SPROP_[SG]ET
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.

Updated

19 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 19 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.

Updated

19 years ago
Status: RESOLVED → VERIFIED

Comment 49

19 years ago
Verified on 1999-05-18-09 build on Windows NT.
bulk reassigning all bugs from shaver's @netscape account to his @mozill acount

Updated

19 years ago
Status: RESOLVED → VERIFIED

Comment 51

19 years ago
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.