1037519 - inIDOMUtils.selectorMatchesElement should allow matching pseudo-elements

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This is needed for bug 777674.

The basic idea is that we should add an [optional] in DOMString aPseudo argument to selectorMatchesElement.  Then in the implementation we should atomize if non-empty and then replace this block:

378   if (sel->mSelectors->IsPseudoElement()) {
379     *aMatches = false;
380     return NS_OK;
381   }

with first checking for cases when sel->mSelectors is a pseudo-element but no pseudo was passed in or vice versa and returning not matches.  Then we want to make sure the passed-in pseudo matches the pseudo in sel->mSelectors, in the case when a pseudo is passed in.  This can be done by comparing sel->mSelectors->PseudoType() to nsCSSPseudoElements::GetPseudoType() for the given pseudo atom.

For now we can probably ignore pseudo-classes on the pseudo-element itself (in practice that's stuff like :hover, which is pretty unreliable in terms of UI display anyway, I'd think).  So what we want to do at that point is update the nsCSSSelectorList to start with the selector after the pseudo-element one.  Probably we should add an API on nsCSSSelectorList to do that (basically grab mSelectors, set mSelectors to mSelectors->mNext, set the old mSelectors->mNext to null and then free the old mSelectors).

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: selectorMatchesElement.patch

I've started as you've outlined in the description and added a test.  I had to include the changes as you mentioned in the final paragraph in the description to get it to work at all - otherwise it was still passing the pseudo element into SelectorListMatches.  I've added a function called RemoveCurrentSelector on nsCSSSelectorList to handle this - should I rename it to something else or change the interface somehow?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8455509 [details] [diff] [review]
selectorMatchesElement.patch

>+++ b/layout/inspector/inDOMUtils.cpp
>+  if (aPseudo.IsEmpty() && sel->mSelectors->IsPseudoElement()) {

I think this should be:

  if (aPseudo.IsEmpty() != sel->mSelectors->IsPseudoElement())

because otherwise if a bogus pseudo-element name is passed and the selector has no pseudo-element selector in it we'll get "not pseudo-element" for both nsCSSPseudoElements::GetPseudoType() and sel->mSelectors->PseudoType() and proceed to incorrectly remove parts of the selector, etc.  We should add a test for this case.

Alternately, we could either return false or throw directly if nsCSSPseudoElements::GetPseudoType(pseudoElt) returns "not pseudo-element" in the !aPseudo.IsEmpt() case.

>+    // We have a matching pseudo element, now remove it so we can compare directly
>+    // against element when proceeding into SelectorListMatches.
>+    sel->RemoveCurrentSelector();

We should also mention in the comment that this is OK to do because we just cloned sel, so no one else is pointing to it.

>+++ b/layout/style/StyleRule.cpp
>+nsCSSSelectorList::RemoveCurrentSelector()

Let's name this "RemoveRightmostSelector".

>+  nsCSSSelector* current = mSelectors;
>+  mSelectors = mSelectors->mNext;

MOZ_ASSERT here that mSelectors is not null after this, please.  And document in the header that this method should only be used on a list that has more than one selector in it.

Also, need to mSelectors->SetOperator(char16_t(0)) here, I believe.

>+  current->mNext = nullptr;

Please document that this is needed so that deleting current won't delete the whole list.

>+++ b/layout/style/StyleRule.h
>+   * Point |mSelectors| to its |mNext|.

... and delete the first node in the old mSelectors.

r=me with those nits

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> Comment on attachment 8455509 [details] [diff] [review]
> selectorMatchesElement.patch
> 
> >+++ b/layout/inspector/inDOMUtils.cpp
> >+  if (aPseudo.IsEmpty() && sel->mSelectors->IsPseudoElement()) {
> 
> I think this should be:
> 
>   if (aPseudo.IsEmpty() != sel->mSelectors->IsPseudoElement())
> 
> because otherwise if a bogus pseudo-element name is passed and the selector
> has no pseudo-element selector in it we'll get "not pseudo-element" for both
> nsCSSPseudoElements::GetPseudoType() and sel->mSelectors->PseudoType() and
> proceed to incorrectly remove parts of the selector, etc.  We should add a
> test for this case.
> 
> Alternately, we could either return false or throw directly if
> nsCSSPseudoElements::GetPseudoType(pseudoElt) returns "not pseudo-element"
> in the !aPseudo.IsEmpt() case.

I think the early check makes sense and does indeed cover the issue more simply than checking once inside the !isEmpty case, though the check should be ==.

    // Do not attempt to match if a pseudo element is requested and this is not
    // a pseudo element selector, or vice versa.
    if (aPseudo.IsEmpty() == sel->mSelectors->IsPseudoElement())

I've added a check in the test for calling with a bogus pseudo element.

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: selectorMatchesElement.patch

Addresses feedback.  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b36bb4cd43ee

Comment 5

[Boris Zbarsky [:bzbarsky]]

> though the check should be ==

Ah, indeed good catch.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cda8d9d66ba

Comment 7

[Gary [:streetwolf52]]

This patch, based on regression checking, is causing Fx33 to crash presumably when there is a Flash video on a site. I'm running Windows 8.1 update 1 x64.  www.cnn.com is an example of a site that crashes for me. Here's what my event log has:

Faulting application name: plugin-container.exe, version: 33.0.0.5310, time stamp: 0x53cc12c9
Faulting module name: mozalloc.dll, version: 33.0.0.5310, time stamp: 0x53cb2a75
Exception code: 0x80000003
Fault offset: 0x000012d5
Faulting process id: 0x193c
Faulting application start time: 0x01cfa450ab8edb7b
Faulting application path: C:\Users\Gary\My Programs\Firefox\plugin-container.exe
Faulting module path: C:\Users\Gary\My Programs\Firefox\mozalloc.dll
Report Id: e9bebb8d-1043-11e4-83a4-c86000a0a026
Faulting package full name: 
Faulting package-relative application ID:

Comment 8

[Gary [:streetwolf52]]

Good:  
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc53a345824f

Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cda8d9d66ba

Comment 9

[Gary [:streetwolf52]]

Don't know if the crash report is very helpful because of a lack of debug info but here it is:

https://crash-stats.mozilla.com/report/index/79dce63e-fd91-41e7-8113-7bb192140720

Comment 10

[Gary [:streetwolf52]]

I did some further testing and found out that the add-on I use, FlashStopper, creates the crash much quicker than without it.  With FS disabled the crash most often happens when I exit Fx33.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1cda8d9d66ba

Comment 12

[Boris Zbarsky [:bzbarsky]]

Gary, can you please file a new bug on that crash?  It's likely to take some investigating.  :(

Comment 13

[Boris Zbarsky [:bzbarsky]]

In particular, FlashStopper doesn't use this API, has no binary bits I can see, and I can't reproduce the crash (on Mac, though).

Comment 14

[Sebastian Zartner [:sebo]]

I am not able to reproduce this crash on a Win7 32-bit system either.

Wanted to note that this new functionality needs to be documented here:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/inIDOMUtils

Sebastian

Comment 15

[Brian Grinstead [:bgrins]]

> Wanted to note that this new functionality needs to be documented here:
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/inIDOMUtils

There was no section for selectorMatchesElement previously, so I've added one and also documented the new parameter: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/inIDOMUtils#selectorMatchesElement()

Comment 16

[Gary [:streetwolf52]]

It appears my crashes were caused by a pref although I don't know which one. I started out with a clean prefs.js and manually added my add-ons entries to it.  All seems fine now.