303260 - if there is no root element, keyboard shortcuts and the context menu do not work [@ nsContentList::nsContentList]

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Grant Wohl]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050802 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050802 Firefox/1.0+

If there is no root element, keyboard shortcuts and the context menu do not work.

Reproducible: Always

Steps to Reproduce:
1.Load any XHTML/HTML/XML document.
2.Use the DOM Inspector to remove the root element(usually "html"), or use a
bookmarklet:<javascript:void(document.removeChild(document.getElementsByTagName("*")[0]))>
3.Try to use a keyboard shortcut (e.g., CTRL+W) or try to bring up the context menu.

Actual Results:  
Nothing happenend.

Expected Results:  
Either the keyboard shortcut action should have happened or the context menu
should have been brought up.

Comment 1

[Grant Wohl]

If you remove "document.documentElement" the same thing occurs.

Comment 2

[Grant Wohl]

Attachment: HTML Testcase

Comment 3

[Aaron Leventhal]

Why would anyone ever remove the root element?

Comment 4

[Grant Wohl]

(In reply to comment #3)
> Why would anyone ever remove the root element?

I don't know.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Why would anyone ever remove the root element?

Why wouldn't they?  We shouldn't break when it happens.

Comment 6

[Hixie (not reading bugmail)]

It's not a "major" bug, IMHO. But it is a valid bug.

Comment 7

[Boris Zbarsky [:bzbarsky]]

It's pretty major for keyboard-only users (see "access" keyword).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Furthermore, this bug is forcing us to have buggy and insecure code elsewhere in the product to work around it.  See bug 341730.

Aaron, Mats, is the issue that nsEventStateManager::ShiftFocusInternal screws this up or something?

Comment 9

[Mats Palmgren (inactive)]

I'll have a look...

I see reproducable crash for the following:
1. start seamonkey
2. click on throbber
3. load the URL from this bug
4. click on throbber -> crash

0x41204bfb in nsContentList (this=0x88fa710, aRootNode=0x0, aMatchAtom=0x80af380, aMatchNameSpaceId=-1, aDeep=0) at nsContentList.cpp:346
346       mRootNode->AddMutationObserver(this);
(gdb) p mRootNode
$1 = (class nsINode *) 0x0

Before the crash I see:
###!!! ASSERTION: content list has to have a root: 'aRootNode', file nsContentList.cpp, line 248
###!!! ASSERTION: Must have root: 'mRootNode', file nsContentList.cpp, line 339

Comment 10

[Mats Palmgren (inactive)]

Attachment: crash stack

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hmm... So is the problem that Destroy() has already been called so that OnPageHide has a null mRootContent?

Note that the crash is a separate bug from what this bug is actually about; it indicates that either OnPageHide is broken as designed or that we really do need to not null out mRootContent in nsDocument::Destroy.  Please file a separate bug on that?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Some indication of what's up

The presshell parts of this fix this bug for me, as far as I can tell.

The docshell parts make tabbing to the rootless document focus its content area; not sure whether we even want that, since any key will blur it, per the presshell code.

Comment 13

[Mats Palmgren (inactive)]

Attachment: Patch A rev. 1 (fixes the crash)

(In reply to comment #11)
> Hmm... So is the problem that Destroy() has already been called so that
> OnPageHide has a null mRootContent?

It was RemoveChildAt() that made mRootContent null:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/base/src/nsDocument.cpp&rev=3.663&root=/cvsroot&mark=1869,1883#1863

This patch fixes the crash at least. I also reviewed the rest of
nsDocument.cpp and it looks mRootContent-null-safe AFAICT.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 229521 [details] [diff] [review]
Patch A rev. 1 (fixes the crash)

Ah, right.  This looks good.  Thanks!

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 229521 [details] [diff] [review]
Patch A rev. 1 (fixes the crash)

Checked in to trunk at 2006-07-18 20:53 PDT.

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 229517 [details] [diff] [review]
Some indication of what's up

So, should we take only the pres shell bits for now?
r=me on both changes (FWIW), but I think only pres shell is needed here.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I really have no idea; I don't know the code well enough to tell.

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 229517 [details] [diff] [review]
Some indication of what's up

Looks good. r=mats

Comment 19

[Boris Zbarsky [:bzbarsky]]

Checked in the presshell and docshell fixes too.  This puppy is fixed.