1282787 - In a XULDocument, document.querySelectorAll incorrectly returns elements in XBL (and native anonymous?) subtrees when the selector has an ID

Status: RESOLVED WONTFIX

(Core :: DOM: CSS Object Model, defect, P3)

Description

[:Gijs (he/him)]

Here's a fun one...

STR:

0. open nightly
1. open the identity popup by clicking the identity block next to the URL, that tells you the security state of the current page.
2. open browser console (if you haven't, enable input in the normal web dev tools' options by enabling 'chrome and add-on debugging')
3. run these two expressions:

a)
document.querySelector("#identity-popup-mainView")
// this clearly shows that the element exists and has a mainview=true attribute

b)
document.querySelector("[mainview=true]")
// this returns null.

expected is that both return the same thing (either both null or both no element).

Note that after step (1), we've appended the element in question into an anon part of the panelUI.xml XBL binding, which I'm sure is relevant (it also doesn't have the 'mainview' attribute before that).

Olli, can you help explain what's going on here? It's messing with my head.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

dbaron could probably answer to this faster.

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Presumably this fast path:
http://searchfox.org/mozilla-central/rev/ef24c234ed53b3ba50a1734f6b946942e4434b5b/dom/base/nsINode.cpp#2826
is in some way searching a different set of elements.  This dates back to https://hg.mozilla.org/mozilla-central/rev/b068485e4cc26cf7ad908e312bd1f47c50595098 (bug 696205).

I think this is really a question for bz...

Comment 3

[:Gijs (he/him)]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #2)
> Presumably this fast path:
> http://searchfox.org/mozilla-central/rev/
> ef24c234ed53b3ba50a1734f6b946942e4434b5b/dom/base/nsINode.cpp#2826
> is in some way searching a different set of elements.  This dates back to
> https://hg.mozilla.org/mozilla-central/rev/
> b068485e4cc26cf7ad908e312bd1f47c50595098 (bug 696205).
> 
> I think this is really a question for bz...

Yes, and I would have asked but he's off on PTO.

I haven't checked if this applies to web shadow DOM, and/or how much would break if we were to now change the two options to be consistent... :-\

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I think my conclusion is that:

 * the #id optimization is buggy

 * it has an nsContentUtils::ContentIsDescendantOf check, but that doesn't do the right thing for XBL or for native-anonymous content (but it should be right for shadow DOM).

 * we should have a version of nsContentUtils::ContentIsDescendantOf that checks for NODE_IS_ANONYMOUS_ROOT and bails out of the loop.  I think it looks like that will do the right thing for both XBL and native-anonymous content.

   * but we should make sure that we handle ShadowRoot.querySelector(All) correctly, and don't bail out every time one element too early!

 * we should have tests of querySelector(All) trying to cross XBL, native anonymous, and shadow DOM, both with and without the ID optimization, and calling querySelector on document, element, and (only for the shadow DOM cases) ShadowRoot.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I won't have time really to spend on this (and really shouldn't have been reading the mail for this bug to start with), but one quick note before people spend too much more time on this and write code that I'd r-: on the web, the #id optimization is totally correct.  The reason for that is that it uses the id table on the document, which for non-XULDocument documents never includes anonymous content (otherwise getElementById would also be buggy, obviously).  See the logic in Element::AddToIdTable which skips doing it for anon content.

Now in a XULDocument, things go all sideways because XULDocument::AddElementToDocumentPre just throws everything in the id table, including anonymous content.  And it's called on all inserts via XULDocument::AddSubtreeToDocument, as well as being called during XUL prototype instantiation and XBL binding instantiation.  This means that getElementById in a XULDocument _will_ return anonymous content.  I'm sure things depend on that.

So as I see it our options are:

1)  Leave everything as-is.
2)  Change querySelector(All) in XULDocument to never return anonymous content
    via the id optimization.  There are various ways of doing this which have
    various levels of performance suck; it's very important that we do not slow
    down the sane non-XULDocument case.  If we do this, querySelector and
    getElementById will have different behavior in XULDocument.  Of course they
    already do to some extent, because of the whole @ref thing in XUL
    and the way XULDocument::getElementById handles it.
3)  As #2 but also change getElementById.  I'm pretty sure this would break the world.

I do agree we should have tests for all the cases covered by the fourth bullet point in comment 4.  We just need separate copies of them for XULDocument and sane documents....

Comment 6

[Andrew Overholt [:overholt]]

(Feel free to disagree with P3 (aka backlog)).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Gijs, thoughts on the options from comment 5?

Comment 9

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Gijs, thoughts on the options from comment 5?

I think we should pick option 1 and wontfix this (and potentially document this somehow).

It feels like getElementById() and querySelector(All) should be consistent, at least, and I agree too much stuff would break if we changed getElementById. I even expect too much stuff to break if we change just querySelector(All) to be inconsistent with getElementById.

On top of that, XUL is dying, so investing in it by trying to make this behaviour sane seems like lots of work for little to no benefit, with the breakage downside also for mothballed/maintenance-mode things like some add-ons, Thunderbird, etc. - quite apart from having to find out the hard way exactly how much breaks in Firefox itself. The hard way because there are too many getElementById()/querySelector(All) calls to audit in a reasonable timeframe (and too much DOM that actually changes at runtime which makes auditing hard, cf bug 1281493 which led me to file this), and not enough automated tests that would actually reveal bustage on try.

Is wontfix OK from your perspective?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yep, totally OK from my perspective, and I completely agree with your reasoning regarding likely breakage and limited upsides.