1335303 - stylo: Gecko_MatchesElement triggers non-threadsafe QI of Element to nsIMozBrowserFrame

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

See bug 1294915 comment 32.

Comment 1

[Bobby Holley (:bholley)]

We can solve this with a type check and a static_cast.

Comment 2

[Bobby Holley (:bholley)]

Error: AddRef/Release on nsISupports
Location: nsCOMPtr<T>::~nsCOMPtr() [with T = nsIMozBrowserFrame] @ ff-dbg/dist/include/nsCOMPtr.h:404
Stack Trace:
static mozilla::Maybe<bool> nsCSSPseudoClasses::MatchesElement(nsCSSPseudoClasses::Type, const mozilla::dom::Element*) @ layout/style/nsCSSPseudoClasses.cpp:153
Gecko_MatchesElement @ layout/style/ServoBindings.cpp:208

Comment 3

[Bobby Holley (:bholley)]

Actually, I'm a bit fuzzy on whether we can just trust the NodeInfo and static_cast.

smaug, what's the best way to determine if something is an nsGenericHTMLFrameElement without QIing?

Comment 4

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

node type + namespace id + localName atom should be safe.
http://searchfox.org/mozilla-central/rev/f31f538622d0dfb2d869d4babc4951f6001e9be2/dom/base/nsINode.h#616

So in nsGenericHTMLFrameElement case you'd need to check frame/iframe atoms.
But perhaps better to add a virtual helper method to nsINode or somewhere and override it in 
nsGenericHTMLFrameElement.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Add a virtual method to avoid QI to nsIMozBrowserFrame. v1

Comment 6

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

Comment on attachment 8833066 [details] [diff] [review]
Add a virtual method to avoid QI to nsIMozBrowserFrame. v1

>   /**
>+   * Returns true if the element is a frame or iframe element.
certainly it doesn't, since if returns nsIMozBrowserFrame* not boolean ;)

>+   *
>+   * We have this method, rather than using QI, so that we can use it during
>+   * the servo traversal, where we can't QI DOM nodes because of non-thread-safe
>+   * refcounts.
>+   */
>+  virtual nsIMozBrowserFrame* AsFrameElement() { return nullptr; }
Please don't call this AsFrameElement, but AsMozBrowserElement.
And maybe even GetAsMozBrowserElement() to hint that it may return null


>       return Some(browserFrame && browserFrame->GetReallyIsBrowser());
Not about this bug, but
GetReallyIsBrowser() isn't safe to be called from other threads when main thread is running. Is it guaranteed that main thread is stopped here?
And hmm, BrowserFramesEnabled() needs to be called once in main thread before this code runs.

But r+ adding the method

Comment 7

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 8833066 [details] [diff] [review]
> Add a virtual method to avoid QI to nsIMozBrowserFrame. v1
> 
> >   /**
> >+   * Returns true if the element is a frame or iframe element.
> certainly it doesn't, since if returns nsIMozBrowserFrame* not boolean ;)

Good catch.

> 
> >+   *
> >+   * We have this method, rather than using QI, so that we can use it during
> >+   * the servo traversal, where we can't QI DOM nodes because of non-thread-safe
> >+   * refcounts.
> >+   */
> >+  virtual nsIMozBrowserFrame* AsFrameElement() { return nullptr; }
> Please don't call this AsFrameElement, but AsMozBrowserElement.
> And maybe even GetAsMozBrowserElement() to hint that it may return null

I'll do GetAsMozBrowserFrame(), since that most closely matches the type (and doesn't get us into the weeds about whether it's "really" a moz browser element).

> >       return Some(browserFrame && browserFrame->GetReallyIsBrowser());
> Not about this bug, but
> GetReallyIsBrowser() isn't safe to be called from other threads when main
> thread is running. Is it guaranteed that main thread is stopped here?

Yes. None of this would be safe if it weren't.

> And hmm, BrowserFramesEnabled() needs to be called once in main thread
> before this code runs.

Good catch! I will add code to do that before the servo traversal.

> 
> But r+ adding the method

Comment 8

[Bobby Holley (:bholley)]

Attachment: Explicitly set up the bool var cache for BrowserFramesEnabled. v1

Comment 9

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98924c761ac4cda411cc07c992fbddb37081ed6d

Comment 10

[Bobby Holley (:bholley)]

And an m-c try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eceeed6f236ca1cf2b34c4dfc7e07ae4552bab4a

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/321ea09ee897
Add a virtual method to avoid QI to nsIMozBrowserFrame. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/fadf7c2a257f
Explicitly set up the bool var cache for BrowserFramesEnabled. r=smaug

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/321ea09ee897
https://hg.mozilla.org/mozilla-central/rev/fadf7c2a257f