350429 - Trailing "}" fails to log syntax error

Status: VERIFIED WORKSFORME

(Core :: XBL, defect)

Description

[Phil Ringnalda (:philor)]

Local backout of bug 350268 says that Firefox trunk is currently failing to open the Places version of the bookmark manager due to a trailing "}" somewhere, but there's no "syntax error" logged to the console, only a couple of (probably unrelated and I think longstanding)

Error: this.treeBoxObject has no properties
Source File: chrome://browser/content/places/tree.xml
Line: 35

No idea how it's evading the syntax error logging, since I haven't found the triggering "}" yet, but Brendan said to file the failure to report failure.

Comment 1

[Phil Ringnalda (:philor)]

Or, perhaps not unrelated: in my backed-out version, those errors don't appear.

Comment 2

[Brendan Eich [:brendan]]

I don't think this is a JS engine bug -- it's more likely something in our many-layered system failing to report what the JS engine threw, or what it reported via the errorReporter callback if uncaught.

/be

Comment 3

[Brendan Eich [:brendan]]

Phil, if you are game to debug with the patch for bug 350268 re-applied to your tree, and break in js_CompileFunctionBody where TOK_EOF is not matched after the FunctionBody is parsed, that would help a lot.

/be

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, a stack to that would rock.

Comment 5

[Phil Ringnalda (:philor)]

This is likely to be beyond me: the "}" that was triggering the error, but not the logging, was in toolkit/content/widgets/tree.xml, and apparently in a static build the treeBoxObject error got in the way (see also bug 350435 where DOMi was saying this.mDOMTree.treeBoxObject has no properties), but while Brendan was failing to make me smart enough to use gdb, I noticed that in a debug build the syntax error from tree.xml did make its way through to the console, so now step one to reproduce is to reverse attachment 235718 [details] [diff] [review], and step two is probably to use a static build.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Where in tree.xml?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #6)
> Where in tree.xml?

It was fixed in bug 350407.

Comment 8

[Brendan Eich [:brendan]]

See http://pastebin.mozilla.org/218, Phil kindly pasted.  This is due to weakness in lxr.mozilla.org/mozilla/ident?i=XBL_ProtoErrorReporter.

/be

Comment 9

[Phil Ringnalda (:philor)]

Attachment: Stack

The full, but probably useless, stack. In the cold light of day, I think what I'm actually reporting is the either uninteresting or invalid by-design fact that XBL compilation errors aren't reported in optimized builds: in a debug build I do wind up with the syntax error in the console, it's only in an optimized build that you get fallout errors from broken trees without the actual syntax error in the tree binding that caused the trees to not exist.

Comment 10

[Boris Zbarsky [:bzbarsky]]

What weakness?  I tried creating a testcase wherein an XBL method has a trailing '}' and I get a syntax error logged, no matter what I do.

Phil, not reporting XBL compilation errors in opt builds is not by-design and definitely a serious bug...

Comment 11

[Boris Zbarsky [:bzbarsky]]

As in, do we know _why_ opt builds don't report?

Comment 12

[Brendan Eich [:brendan]]

I thought that error reporter was giving up too easily -- is it possible that the console service wasn't available when the error was reported, early in startup?

/be

Comment 13

[Boris Zbarsky [:bzbarsky]]

Console service is a base XPCOM service; once libxpcom has been loaded, it's available.  And the problem originally reported was in the bookmark manager, which isn't being opened at startup anyway.

So no, startup timing is not likely an issue here.  Again, I'd love to have a testcase, or even just a way to reproduce this locally.

Comment 14

[Phil Ringnalda (:philor)]

(In reply to comment #13)
> And the problem originally reported was in the bookmark manager,
> which isn't being opened at startup anyway.

The bookmark manager was having trouble because of its use of trees, which didn't exist because the tree widget's binding blew up while being compiled at startup. In a debug build, you get a syntax error from that in the console without needing to open the bookmark manager, in an opt build you don't see any errors logged until the bookmark manager tries to access properties of its trees and finds they don't exist.

My steps to get the stack I attached were to put back the extra } that attachment 235718 [details] [diff] [review] took out, and set a breakpoint in jsparse.c:814. That then gets triggered during startup, though I'm clueless about when other than "long before any window starts to appear," and also clueless about what to look for while stepping through after that, to see where debug builds do log to the console and opt builds do not.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Interesting.  I'd expect the binding to be compiled when it's used; perhaps something in the Firefox window uses trees....  I'll look into this as soon as I get an optimized firefox build to not segfault on startup.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase that shows that XBL reports this error just fine

Just download and run; it won't run from bugzilla.

I have no idea what philor is talking about with opt builds; the attached testcase shows XBL reporting the syntax error perfectly fine in an opt Firefox build.

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK, I just tried backing out the patch from bug 350407 locally and seeing what behavior I get when following the steps in that bug.   The syntax error is reported to the console fine, in both debug and opt firefox.  It's not printed on stdout, in both debug and opt firefox.  That makes, sense, since  XBL_ProtoErrorReporter doesn't do the debug-only printf stuff that the DOM reporter does.

So this is completely worksforme.  Please reopen if you have actual steps to reproduce.

Comment 18

[Phil Ringnalda (:philor)]

Yup. I started doing some pull-by-dates on 1.9.0 to see when it started working, but then I realized I didn't actually care that much.