50267 - Ill-formed browserBindings.xul crashes mozilla

Status: CLOSED INVALID

(SeaMonkey :: UI Design, defect, P1)

Description

[timeless]

<key is="emHelpKey" keycode="VK_F1" control="false" shift="false"
        onkeypress="
            window.delayedOpenWindow( getBrowserURL(), 'all,dialog=no", 
'http://www.mozilla.org/projects/seamonkey/release-notes/');
            "/>
the problem is 'all,dialog=no" [mismatched quotes].

Talkback incidents <dbaron> timeless:  16272910, 16273350, 16273362, 16273781, 
16274230 are the keybindings crash

Comment 1

[saari (gone)]

I just pass that quoted string to JS for interpretation. This is not an
XPtoolkit bug.

Comment 2

[Peter Trudelle]

I'm stumped.  Who should get it?

Comment 3

[Peter Trudelle]

Sounds like JS should be flagging an error rather than crashing, but the
expedient thing to do for N6 is fix the xul file. ->xpapps.

Comment 4

[timeless]

Careful, if I were parsing id have:
<key
is="emHelpKey"
keycode="VK_F1"
control="false"
shift="false"
onkeypress="window.delayedOpenWindow(...=no"
, 'http://www.mozilla.org/projects/seamonkey/release-notes/');"/>

Assuming that's the case i wouldn't think it's the javascript itself that is 
messing it up, but the last line ...

Maybe parser?  It depends exactly what is messing up.

Comment 5

[timeless]

Erm i take that back (and run to dinner)
we crash when i hit the key, so the js is executing.
window.delayedOpenWindow(getBrowserURL(), 'all,dialog=no
so the js engine needs to check for matched parens or whatnot ..
sorry

Comment 6

[sairuh (rarely reading bugmail)]

timeless, what are your steps for causing this crash? thx.

Comment 7

[timeless]

There are lots of ways to crash, so i'll list them here [run moz, press f1 
from a gui, eg Profile Manager]:
<key is="emHelpKey" keycode="VK_F1" control="false" onkeypress="alert(", 
'');"/>
<key id="emHelpKey" keycode="VK_F1" onkeypress="alert(", '');"/>
<key id="emHelpKey" keycode="VK_F1" "/>
<key id="emHelpKey" keycode="VK_F1" '';/>
<key id="emHelpKey" keycode="VK_F1";/>
<key id="emHelpKey" keycode="VK_F1" a/>
<key id="emHelpKey" a keycode="VK_F1"/>
<key id="emHelpKey" a= keycode="VK_F1"/>
<key id="emHelpKey" keycode="VK_F1" a="", />
<key id="emHelpKey" keycode="VK_F1" & />
<key id="emHelpKey" keycode="VK_F1" ? />
<key id="emHelpKey" keycode="VK_F1" 1 />
<key id="emHelpKey" keycode="VK_F1" 1="2" />
<key id="emHelpKey" keycode="VK_F1" \ />

JavaScript error: line 0: syntax error
<key is="emHelpKey" keycode="VK_F1" control="false" onkeypress="alert("/>
This does not crash:
<key id="emHelpKey" keycode="VK_F1"/>
<key id="emHelpKey" a="" keycode="VK_F1"/>
<key id="emHelpKey" keycode="VK_F1" oncommand="alert('hi')" />

behaves correctly:
<key id="emHelpKey" keycode="VK_F1" onkeypress="alert('hi')" />

--
FWIW, closing the profile manager dialog (x) seemed to crash moz too. Maybe I 
should get another build :|
--
Conclusion: It isn't JavaScript's fault (my first guess just might have been 
right :o)

Comment 8

[saari (gone)]

Is the stack on all of those crashes the same? What is it? 

Comment 9

[timeless]

Stack? what's a stack?
Due to a feature of QFA I crash before QFA is even loaded. Once i get on campus 
I can try using current debug builds (actually i could just use solaris, but 
I'm busy, I should be doing work instead of QA)
I'm pretty sure the stacks /would/ be the same if I  could see them...

Comment 10

[Claudius Gayle]

nav traige team: [NEED INFO] Is this still reproduceable?

Comment 11

[Asa Dotzler [:asa]]

I see this crash on 082908 mozilla win32 talkback build on NT4.  I launch
mozilla -ProfileManager and click the X to close it and crash.  

Comment 12

[Asa Dotzler [:asa]]

Tested this with 082915 mozilla Mac build on OS9.  I inserted that first snippet 
into platformBrowserBindings.xul and started mozilla.  hitting F1 lcoked the app 
up completely.  had to kill the unresponsive app with command option escape.

Comment 13

[sairuh (rarely reading bugmail)]

fwiw, i wasn't able to get this crash on linux following asa's steps using
-ProfileManager.

Comment 14

[Claudius Gayle]

nav triage team: assigning over to Profile Manager Frontend. We're also 
perplexed. Is it that someone is trying to add this keybinding(F1) and this has 
exposed a crash, or what?

Comment 15

[Claudius Gayle]

nav triage team: d'oh didn't realize ben was the owner for Profile manager 
frontend too. Anyway, we [NEED MORE INFO]. How does the keybinding(F1) issue 
relate to crashing the profile manager when clicking the close box (which we 
were able to repro)? Is this two separate bugs?

Comment 16

[Grace Bush]

close box bug is bug 49615

Comment 17

[timeless]

This bug was just misfiled. F1 will crash at any point. I just use profile 
manager because it loads fast.

The crashing by clicking the close bug is just another bug. I'm sorry I 
mentioned it.

Comment 18

[Peter "jag" Annema]

So basically, this is a 'make xul/xml parser more robust' bug?
Not necessarily make it "smarter" (we've seen html, not again thanks), just make 
it not crash when encountering bad/incorrect/corrupted xul/xml?

Comment 19

[johng]

nav triage team:
assuming everytime you click F1 on Windows 2000 causes a crash, this is a
nsbeta3+ P1.

Changing component to XP Apps.

Comment 20

[Ben Goodger (use ben at mozilla dot org for email)]

I cannot find the code displayed in the file that this refers to, nor reproduce
this crash as no steps were supplied. 

Comment 21

[timeless]

Ben. pick any of the strings from  [e-mail] 2000-08-26 22:46
copy it into the browserBindings file.  Run mozilla and press F1.  I think this 
belongs to hyatt.

Comment 22

[Ben Goodger (use ben at mozilla dot org for email)]

so this is nothing to do with anything in the tree? 

removing nsbeta3+ then. 

Comment 23

[johng]

nav triage team:
Claudius is creating a new bug for F1 causes crash, and we will + that one.
This one gets a minus.

Comment 24

[johng]

F1 Help crasher bug 51908

Comment 25

[Dan Rosen]

Invalid in the context of XML means "not conforming to DTD." Ill-formed, meaning
"not conforming to XML syntax" is what you mean. Mismatching quotes causes
ill-formedness. The XML parser should not allow ill-formedness, as that would
make our XML parser non-conformant :)

In other words, just fix the damn quotes. And if this isn't even in the tree,
then why is this bug still open?

Comment 26

[Peter "jag" Annema]

Well, this could just be morphed into "[RFE] Make Mozilla not crash on 
ill-formed xul/javascript" :-)

Comment 27

[Dan Rosen]

jag: you don't want that. trust me. you just really don't want that.

everybody and their brother write ill-formed, downright nasty html. this is only
encouraged by the fact that all browsers accept it. the same situation should
not occur with xml -- xml parsers are very easy to use: just as html authors
test their pages in a browser, xml authors can test their stuff in mozilla or
whatever other new generation browser they use...

also, we do not crash on ill-formed xml. we crash on F1 on win2k (bug 51908).

the short story is: xul is xml. xml is supposed to be well-formed. parsers that
accept ill-formed xml are non-conformant. we are conformant. write correct xul.

Comment 28

[Peter "jag" Annema]

dr: From what I understood from talks with timeless, mozilla _was_ crashing on
invalid xul, the actual key used didn't matter. I may have misunderstood that
though.

Anyway, I agree with you on invalid/ill-formed xml, so I think you're
misunderstanding me here.

If mozilla encounters invalid or ill-formed XML, it should make the user aware
of that, perhaps through an error message, perhaps by just not doing anything,
I'll let others think up something clever on that. If it's invalid XUL in the
chrome, it's even more important the developer is made aware of the error.
Mozilla should _never_ try to guess what was meant. But, it should _not_ crash.
Though, that is a good way to get someone's attention and get the XUL fixed ;-)

Catch me in irc if things are still unclear.

Comment 29

[Dan Rosen]

jag: ah, now i understand. the value of clarity is high...

anyway, feel free to open a new bug re: ill-formed xul crashing mozilla. i'll
bet it's just a case of a component not anticipating a strange error case (like,
the xml parser cleanly handling the error, but the xul impl not understanding
it...) just double-check to see if it's still a problem (by breaking a xul file
or something).

somebody care to verify this bug's resolution?

Comment 30

[John Morrison]

Marking verified invalid, if for no other reason than it would be more 
effective to file new bugs for specific cases where where crash on "bad XUL",
instead of morphing this bug for that purpose.

Clearly, it is preferable to flag an error, and abort loading when loading
XUL that is either (1) not well-formed XUL/XML, or (2) well-formed, but invalid
XUL (e.g., elements placed where they should not be placed). 

Comment 31

[timeless]

Thank you for trashing this bug</sarcasm> Please file all of the relevant bugs 
and CC me.

Your resolution is unfair.  I filed this bug w/ the best summary I could, I 
don't speak all dialects of technobabel and am sorry I didn't know the 
difference between invalid and ill formed.

The recommendation you are making is to file more bugs w/ the same summary.

Removing keywords, status. Marking meta.

Would someone please attach the stack traces from the original report?  
Commentary about what's happening according to the stack traces would be 
useful.

Comment 32

[Dan Rosen]

timeless: please re-close this bug as invalid, or open a new bug for ill-formed
xul crashing mozilla, and close it as a dup of that. this bug was originally
misfiled and therefore has too much misinformation to be clear for use in fixing
the bug you *really* want fixed. this is what jrgm and i were talking about, and
has nothing to do with technobabble.

i'm taking this bug myself to make sure you resolve it appropriately.

Comment 33

[Dan Rosen]

A simple test of mangling navigator.xul results not in a crash, but in:

MOZILLA_FIVE_HOME=/home/dr/mozilla/dist/bin
  LD_LIBRARY_PATH=/home/dr/mozilla/dist/bin
    
LIBRARY_PATH=/home/dr/mozilla/dist/bin:/home/dr/mozilla/dist/bin/components      
SHLIB_PATH=/home/dr/mozilla/dist/bin
          LIBPATH=/home/dr/mozilla/dist/bin
       ADDON_PATH=/home/dr/mozilla/dist/bin
      MOZ_PROGRAM=/home/dr/mozilla/dist/bin/mozilla-bin
      MOZ_TOOLKIT=
        moz_debug=0
     moz_debugger=
WEBSHELL+ = 1
WEBSHELL+ = 2
XML Error in file 'chrome://navigator/content/navigator.xul', Line Number: 40,
Col Number: 11, Description: not well-formed

What this shows is that the XML parser does indeed inform the XUL implementation
of parse errors, and that mozilla does not crash, but closes quietly. This bug
is INVALID. If you can find a different case (mangling other XUL files in the
tree) that crashes mozilla, /then/ open a new bug and post the stack trace.

Timeless: in other words, this bug was not marked invalid because I didn't
understand your vagueness or because it was misfiled, but because it was
invalid, and nothing more.

Please be aware that lack of knowledge ("stack? what's a stack?" and "i don't
speak all dialects of technobabble") is no excuse for rudeness, and being
flat-out wrong (this isn't a crasher) doesn't help either. I have a lot of
respect for your seemingly 24x7 commitment to the mozilla project (you spend
more time than I do, and you don't get paid to do this!) but you can't take a
high-and-mighty attitude, or expect that sort of respect, because of your
commitment alone.

Please contact me privately if you feel I'm being unfair, and keep in mind that
it's easy to let your frustration stand in the way of the very same thing you so
strongly commit yourself to.

Comment 34

[timeless]

I'm closing this bug. I know we very much object to closed bugs, but this one 
shouldn't be verified, and i don't want it verified, i want it closed.

Bug 52359 will cause this problem to go away.

This bug was a feature of the loading system we used for the 
(platform)[Browser|Editor]Bindings.xul files.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/rdf/content/src/nsXULKeyLis
tener.cpp&rev=1.77
1253 nsCOMPtr<nsIXULContentSink> sink;
1278 parser->SetContentSink(sink);
1284 rv = parser->Parse(aURI);
1285 if (NS_FAILED(rv)) return rv;
I'm guessing 1284 returns NS_FAILED and that 1285 returns while the sink is 
still in some unhappy state.  But I don't understand nsCOMPtr<>.