219221 - JS exception failing to propagate

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect)

Description

[Alex Vincent [:WeirdAl]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i586; en-US; rv:1.5b) Gecko/20030909
Build Identifier: Mozilla/5.0 (X11; U; Linux i586; en-US; rv:1.5b) Gecko/20030909

Working on a patch for bug 193942, I noticed on Windows a strange failure of the
patch to act properly on certain constant values.  Today, with the JavaScript
Debugger in Linux, I have witnessed a repeatable sequence of steps which causes
the application to suddenly start executing code in the middle of a function
that was not called.

Reproducible: Always

Steps to Reproduce:
 (1) Apply the DOM Inspector patch (located at http://localhost/)
 (2) Start DOM Inspector.
 (3) Use Inspector to inspect itself (File, Inspect a Window, DOM Inspector)
 (4) In DOM Inspector, switch left-hand panel to JavaScript Object viewer.
 (5) Start JavaScript Debugger.
 (6) In JS Debugger, go to the Debug menu and verify the "Exclude Browser Files"
option is unchecked.
 (7) Set breakpoints in JS Debugger as follows:
	* inspector.xml, approx. line 576, reading "var entries =
this.registry.findViewersForObject(aObject, this.id);"
        * ViewerRegistry.js, approx. line 154, reading "if
(this.objectMatchesEntry(aObject, i)) {"

 (8) Select the "ATTRIBUTE_NODE" row in the expanded tree.
 (9) JS Debugger will interrupt at inspector.xml, line 576.  Note call stack
includes setSubject, updateLinkedViewer, and onEvent.
(10) Hit the "Continue" icon.
(11) JS Debugger will interrupt at ViewerRegistry.js, line 154.  Note call stack
includes findViewersForObject, setSubject, updateLinkedViewer, and onEvent.
(12) Use "Step Into" key to advance two lines.  You will arrive at
objectMatchesEntry function, line 176.
(13) Use "Step Into" key one more time.

Actual Results:  
JS Debugger opens a second instance of ViewerRegistry.js, and reports the
controlling line number as 113, in the middle of the "prepareRegistry" function.

Expected Results:  
objectMatchesEntry function ends and passes control back to findViewersForObject
function.

Line 113 says:
fn = new Function("doesQI", "object", js);

The scope for this function has the js variable undefined; that variable is set
two lines earlier.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: Patch for DOM Inspector (testcase, step 1)

I do not know if this testcase is a minimal one or not.  It's the only one I
have.

Comment 2

[Brendan Eich [:brendan]]

Need a reduced testcase, if possible.

/be

Comment 3

[Alex Vincent [:WeirdAl]]

Attachment: Much reduced patch (testcase, step 1)

Reduced testcase:

(1) Apply patch and rebuild mozilla/extensions/inspector
(2) Shut down and restart Mozilla from scratch
(3) Start DOM Inspector.
(4) In Inspector, go to File, Inspect a Window, DOM Inspector
(5) You should see four alert dialogs: two in for XULDocument, two out for the
same document.
(6) Switch the left-hand viewer to JavaScript Object.
(7) Expand the tree that comes up (you do not need to select the first row,
unless you want two more alerts)
(8) Select the "firstChild" entry.  Two more alerts pop up, one in for
XULElement, one out for the same element.
(9) Select the DOCUMENT_POSITION_DISCONNECTED entry.

Expected results:  Two more alerts pop up, one in for the constant, one out for
the same constant.

Actual results: One more alert pops up, in for the constant.  No outbound alert
pops up.

Apologies for the alerts and the corresponding assertions; this is as simple a
testcase as I can come up with without a redesign of Inspector.

Comment 4

[Alex Vincent [:WeirdAl]]

restoring blockage

Comment 5

[Robert Ginda]

I'm betting the wierdness you're seeing in venkman is the interrupt-handler bug
in the js engine, and it only affects what venkman shows you, not what the
engine actually does.  See bug 217768, which I'll check in today.

Comment 6

[Brendan Eich [:brendan]]

Alex, can you close this as a dup of bug 217768 if it indeed was fixed based on
that bug's patch being checked in?

/be

Comment 7

[Alex Vincent [:WeirdAl]]

This bug is present in Mozilla independent of Venkman.

Comment 8

[Phil Schwartau]

Alex: Rob's patch for bug 217768 was for mozilla/js/src/jsinterp.c,
so it affects the JS Engine itself. Perhaps it will fix the problem
you found, too. Are you still seeing it with the fix for bug 217768?

Comment 9

[Robert Ginda]

Actually, I never thought my patch would fix Alex's real problem.  Only the
summary of this bug, which is almost certainly bogus.  The js engine most likely
does not jump to the middle of any function, although that's what it might look
like in venkman before bug 217768.

Now that 217768 is checked in, however, it would be a good idea to try to debug
this script again in venkman and see what is "really" going wrong.

Comment 10

[Alex Vincent [:WeirdAl]]

rginda: After your patch landed, there was ZERO change in the results, including
through Venkman.  I checked to make sure your patch had applied in the source
code I rebuilt last night; it has.  Venkman continues to report a jump to line
113, in the middle of a function as I originally reported.

Comment 11

[Alex Vincent [:WeirdAl]]

Guys, I'm sorry for my outburst in comment 10.  rginda is right in one sense; I
regard it as unlikely that the JS engine would jump lines like that, too.  But
if you examine the structure of the setSubject() method in inspector.xml, you
see there are no returns in the function, and no exceptions or strict warnings
thrown in the JavaScript console or the terminal I use to start Mozilla from
Linux.  Those are the only two logical and correct procedures for exiting a
function before executing all lines of the function which I am aware of.  This
is my basis for filing this bug.

For the setSubject() method in question, 
http://lxr.mozilla.org/seamonkey/source/extensions/inspector/resources/content/inspector.xml#563

Comment 12

[Alex Vincent [:WeirdAl]]

cc'ing folks whom #mozilla said might be able to help.  Anyone with a C++
debugger to help figure out what's going on?  (timeless' idea)

Comment 13

[Andrew Schultz]

with current debug/trunk, I get the 4 alerts in step 5 (comment 3), but I get no
alerts in steps 8 or 9.

Comment 14

[Andrew Schultz]

bad news:
it appears that I screwed up the steps-to-reproduce and was trying the
javascript-object on the right pane instead of the left.  I see the bug as
described on the left pane.

(sorta) good news:
I added copious dump() to setSubject() and have tracked the problem down to here:
http://lxr.mozilla.org/seamonkey/source/extensions/inspector/resources/content/ViewerRegistry.js#181

179   doesQI: function(aObject, aInterface)
180   {
181     if (!("QueryInterface" in aObject)) return false;
182     


for the specific case of DOCUMENT_POSITION_DISCONNECTED (and a few others),
aObject is not an object at all.  It's a number (1).  In that case, the JS
Engine seems to bail without an error.  Even a try/catch block couldn't rescue
execution.

Checking typeof(aObject) before trying to use aObject there is sufficient to
"fix" the behavior here, although it might be better to check that upstream
somewhere.

Comment 15

[Andrew Schultz]

> Even a try/catch block couldn't rescue execution.

try/catch seems to work now, although failure is still silent without it.

also, the following js in inspector.xml's setSubject is sufficient to trigger
the problem:

 function badfunc(aObject) {
   if (!("foobar" in aObject)) return;
 }
 badfunc(aObject);

passing a local variable triggers a JS error.  Failure is silent only when
aObject is passed (and it's a number).

Comment 16

[Alex Vincent [:WeirdAl]]

Attachment: Same testcase patch as attachment 131475, using dump() and a little more detail

Comment 17

[Alex Vincent [:WeirdAl]]

Attachment: Standalone testcase which shows JS exception

Mr. Schultz, I owe you one.

This testcase is an XML file with an inline JavaScript, running barebones
conditions similar to what Mr. Schultz observed in his past two comments.  Note
however that with this testcase, you get a JavaScript exception, and with the
Inspector patch you do not:

Error: invalid 'in' operand 2
Source File: file:///home/ajvincent/219221_test.xml
Line: 5

This suggests a simple fix for the simple bug in ViewerRegistry.js, using
(typeof aObject["QueryInterface"] == "undefined") instead.

At the same time, that doesn't change Mozilla's failure to report the exception
with attachment 131966 [details] [diff] [review] (or attachment 131475 [details] [diff] [review]).	I'd also like some help
identifying just why ("foobar" in aObject) when aObject == 2 throws an
exception in the first place...

Comment 18

[Phil Schwartau]

The exception is also successfully raised in the current JS shell:


js> function badfunc(aObject) {
   if (!("foobar" in aObject)) return;
 }

js> aObject = 1;
1

js> badfunc(aObject);
7: TypeError: invalid 'in' operand 1



The reason for such exceptions is given in the ECMA-262 Ed. 3 spec
at http://www.mozilla.org/js/language/E262-3.pdf. See Step 5 below:

Section 11.8.7 The in operator

The production RelationalExpression : RelationalExpression in ShiftExpression
is evaluated as follows:

1. Evaluate RelationalExpression.
2. Call GetValue(Result(1)).
3. Evaluate ShiftExpression.
4. Call GetValue(Result(3)).
5. If Result(4) is not an object, throw a TypeError exception.
6. Call ToString(Result(2)).
7. Call the [[HasProperty]] method of Result(4) with parameter Result(6).
8. Return Result(7).


This bug is not a JS Engine issue. It's something in the browser
failing to propagate the error that the JS Engine detects - right?

Comment 19

[Alex Vincent [:WeirdAl]]

That's my read on the situation too, Phil.  But who & what component do I
reassign this to?

Comment 20

[Alex Vincent [:WeirdAl]]

reassigning per advice of #mozilla -- jst, can you lend an eyeball or two here?

Comment 21

[Alex Vincent [:WeirdAl]]

Attachment: Standalone testcase demonstrating failure (XUL only)

The exception is not reported from XUL.  However, coming from an XML or HTML
file, it is reported.

This testcase uses an XUL textbox to show the results.

Expected results:
In: [object Object]
Out (true): [object Object]
In: [object Object]
Out (false): [object Object]
In: 2
Out (false): 2

Actual results:
In: [object Object]
Out (true): [object Object]
In: [object Object]
Out (false): [object Object]
In: 2

Comment 22

[Alex Vincent [:WeirdAl]]

Now I know for a fact it's not specifically caused through DOM Inspector.

Comment 23

[Alex Vincent [:WeirdAl]]

Comment on attachment 131966 [details] [diff] [review]
Same testcase patch as attachment 131475 [details] [diff] [review], using dump() and a little more detail

I am smoking some serious crack.  The exception is making it out still with my
133116 attachment.

Comment 24

[Alex Vincent [:WeirdAl]]

I'm giving up trying to figure out where the exception is getting dropped.  I've
filed this bug, and I've tried to reduce the testcase unsuccessfully several
times.  The best thing I can do in this bug is to stop spamming everybody with
ideas that go nowhere, and basically to shut up.

helpwanted: I would very much appreciate having someone take a look at the issue
I'm seeing with the DOM Inspector patch, and a C++ debugger, to track the
exception as it goes from the JavaScript engine.  Where does it go, and why
doesn't it get out to the terminal?

Comment 25

[Andrew Schultz]

Attachment: xul testcase, part 1

load the next one

Comment 26

[Andrew Schultz]

Attachment: xul testcase, part 2

I finally managed to get this testcase out of chrome this morning.

I'm not sure this is actually going to work loading from Mozilla, you might
need to save both parts and point this one at the first file (jsObject.xul) and
load this one locally.

load this file, then click "clickme".  the testcase should dump "looking for
'foobar' in null".  it is then supposed to throw a JS exception, but it
doesn't.

loading from bugzilla, it seems to not execute the setSubject2 function at all.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Loading from bugzilla, I get security exceptions accessing
'UnknownClass.classes'.  I'm not sure what does that access, but I assume it's
XBL somewhere....

Comment 28

[Andrew Schultz]

> Loading from bugzilla, I get security exceptions accessing
> 'UnknownClass.classes'.

I noticed those too.  The same files loaded as chrome don't have the warnings
(but still exhibit the bug)

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

>'UnknownClass.classes'.

if that appears in an error message, it's usually Components.classes, in my
experience...

Comment 30

[Samuel Sieb]

That error seems to usually show up when you try to access chrome stuff from
non-chrome (permission error).

Comment 31

[[e-mail]]

OK, so the reason the debugger is stepping to the line in question is that
that's where the function that excecution is currently stopped at was created. I
leave it to you to investigate what happens when you try to step further into
the function.

By the way, why did nobody spot the horrible code used by doesQI?

    if (!("QueryInterface" in aObject)) return false;
    
    try {
      var result = aObject.QueryInterface(Components.interfaces[aInterface]);
      return true;
    } catch (ex) {
      return false;
    }

When all you need to do is
return aObject instanceof Components.interfaces[aInterface];

Comment 32

[Andrew Schultz]

the testcase is working with current trunk.  possibly fixed by bug 224549?

Comment 33

[Andrew Schultz]

Alex, can you retest this?  I think it's WFM.

Comment 34

[Alex Vincent [:WeirdAl]]

Not for a couple weeks; my Internet connection has temporarily gone down, and I 
won't be back at my desk with it working until the very end of the year.  Sorry.

Comment 35

[Andrew Schultz]

marking WFM.  If this is still a problem, reopen with details.