inIDOMUtils.selectorMatchesElement should allow matching pseudo-elements

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: bzbarsky, Assigned: bgrins)

Tracking

({dev-doc-complete})

unspecified
mozilla33
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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).
Blocks: 777674
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Posted patch selectorMatchesElement.patch (obsolete) — Splinter Review
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?
Attachment #8455509 - Flags: feedback?(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
Attachment #8455509 - Flags: feedback?(bzbarsky) → feedback+
(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.
> though the check should be ==

Ah, indeed good catch.
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:
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
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.
https://hg.mozilla.org/mozilla-central/rev/1cda8d9d66ba
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Gary, can you please file a new bug on that crash?  It's likely to take some investigating.  :(
Flags: needinfo?(garyshap)
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).
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
> 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()
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.
Flags: needinfo?(garyshap)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.