Closed Bug 1335303 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bholley, Unassigned)

References

Details

Attachments

(2 files)

We can solve this with a type check and a static_cast.
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
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?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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
Attachment #8833066 - Flags: review?(bugs) → review+
(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
Attachment #8833083 - Flags: review?(bugs) → review+
Pushed by bholley@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/321ea09ee897
https://hg.mozilla.org/mozilla-central/rev/fadf7c2a257f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.