1434273 - Crash in nsContentUtils::ContentIsDraggable

Status: VERIFIED FIXED

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

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-03e5208c-65be-4222-a63b-1dd390180130.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsContentUtils::ContentIsDraggable dom/base/nsContentUtils.cpp:3810
1 xul.dll nsFrame::HandlePress layout/generic/nsFrame.cpp:4167
2 xul.dll nsFrame::HandleEvent layout/generic/nsFrame.cpp:3913
3 xul.dll nsImageFrame::HandleEvent layout/generic/nsImageFrame.cpp:2130
4 xul.dll nsPresShellEventCB::HandleEvent layout/base/PresShell.cpp:510
5 xul.dll mozilla::EventTargetChainItem::HandleEventTargetChain dom/events/EventDispatcher.cpp:536
6 xul.dll mozilla::EventDispatcher::Dispatch dom/events/EventDispatcher.cpp:859
7 xul.dll mozilla::PresShell::DispatchEventToDOM layout/base/PresShell.cpp:7981
8 xul.dll mozilla::PresShell::HandleEventInternal layout/base/PresShell.cpp:7775
9 xul.dll mozilla::PresShell::HandleEvent layout/base/PresShell.cpp:7390

=============================================================

There are 20 crashes (from are 7 installations) in nightly 60 with buildid 20180130102929. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1432977.

[1] https://hg.mozilla.org/mozilla-central/rev?node=e0759dfa715e

Comment 1

[Boris Zbarsky [:bzbarsky]]

Seems kinda similar to bug 1434315.  Again, the issue is a Draggable() call on nsGenericHTMLElement.  And again I have no idea why this would be crashing.... :(

Comment 2

[Boris Zbarsky [:bzbarsky]]

OK, RyanVM reproduced this and we debugged it together.  We're crashing because aContent is an nsGenConImageContent.  And it's created like so, in nsCSSFrameConstructor::CreateGeneratedContent:


      nodeInfo = mDocument->NodeInfoManager()->
        GetNodeInfo(nsGkAtoms::mozgeneratedcontentimage, nullptr,
                    kNameSpaceID_XHTML, nsIDOMNode::ELEMENT_NODE);

      nsCOMPtr<nsIContent> content;
      NS_NewGenConImageContent(getter_AddRefs(content), nodeInfo.forget(),
                               image);

Note the namespace.  But nsGenConImageContent doesn't inherit from nsGenericHTMLElement!!!

This means every instance of nsGenericHTMLElement::FromContent, or casts to nsGenericHTMLElement is suspect.

Comment 4

[Andrew McCreight [:mccr8]]

I'm calling this "wild pointer" but that's not really right, as there's some fixed type confusion happening between two different specific classes with vtables. It also sounds like this is a larger problem than this specific crash.

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, so auditing casts to nsGenericHTMLElement* we have:

1)  Bindings.  Those are casting things that asked for an HTMLElement binding, so not
    nsGenConImageContent.
2)  nsIDocument::GetDir, which is working on a thing known to have "html" as localName.
3)  nsINode::GetTextEditorRootContent which should never end up under a
    nsGenConImageContent, I think.
4)  EventStateManager::ContentRemoved which checks for "a" or "area" as localName.
5)  HTMLFormElement::BuildSubmission which will never have an nsGenConImageContent as
    originator.
6)  HTMLInputElement::GetList which checks for "datalist" as localName.
7)  HTMLLabelElement::GetLabeledElement which can't end up with nsGenConImageContent
    because those are native anon and unreachable from labels.
8)  HTMLLabelElement::GetFirstLabelableDescendant -- can't reach
    nsGenConImageContent via DOM walk.
9)  HTMLSelectElement::Add -- came from something with an HTMLElement binding, so ok.
10) nsGenericHTMLElement::CopyInnerTo -- aDst better be an nsGenericHTMLElement here.
11) nsHTMLDocument::GetBody -- checks for "body" or "frameset" localName.
12) nsHTMLDocument::SetBody -- removed on tip anyway, but before that was coming
    from a known-HTML thing.
13) nsVideoFrame::CreateAnonymousContent -- we just created it as HTMLDivElement.

For nsGenericHTMLElement::FromContent we have:

1)  ImageAccessible::GetLongDescURI -- checks for "a" or "area" localName.
2)  EditableInclusiveDescendantCount -- Called on "this" in 
    Element::BindToTree/UnbindFromTree!  The Unbind case I'm not sure is reached,
    but I'd expect the Bind case is.  That calls
    nsGenericHTMLElement::EditableInclusiveDescendantCount, but if
    HasFlag(NODE_IS_EDITABLE) is false (which it is here), then we only use
    nsINode methods and hence end up OK.
3)  nsContentList::GetSupportedNames -- ok because nsGenConImageContent never appears
    in content lists.
4)  DocAllResultMatch -- ok because this only looks at non-anonymous content.
5)  HTMLAllCollection::GetSupportedNames -- same thing.
6)  HTMLMenuElement::TraverseContent -- never reaches native anon content.
7)  TableRowsCollection::GetSupportedNames -- never contains nsGenConImageContent.
8)  nsGenericHTMLElement::SyncEditorsOnSubtree -- Only called from
    nsGenericHTMLElement::AfterSetAttr (on "this") and recursively from itself,
    walking the DOM.  So can never reach nsGenConImageContent.
9)  nsGenericHTMLElement::IsContentEditable -- only touches things reachable via parent walk;
    nsGenConImageContent never has descendants, I think.
10) nsLegendFrame::GetLogicalAlign -- poking at mContent, which is always a <legend>.
11) nsTextControlFrame::ShouldInitializeEagerly -- poking at mContent, which is <input>
    or <textarea>.
12) nsTextControlFrame::GetMaxLength -- likewise.
13) nsBulletFrame::SetListItemOrdinal -- poking at the block's content.  Pretty sure this
    can't be a  nsGenConImageContent.  In any case, it only uses Element methods on it.
14) nsContainerFrame::RenumberList -- shouldn't be reached for nsGenConImageContent mContent.
15) nsHTMLFramesetFrame::GetBorderWidth -- poking at mContent, which is a <frameset>.
16) nsHTMLFramesetFrame::GetFrameBorder -- likewise.
17) nsHTMLFramesetFrame::GetFrameBorder (1-arg version) -- has DOM child of <frameset>.
18) nsHTMLFramesetFrame::GetBorderColor -- knows it has a <frameset>.
19) nsHTMLFramesetFrame::GetBorderColor (1-arg version) -- has DOM child of <frameset>.
20) nsSubDocumentFrame::GetMarginAttributes -- can't have an nsGenConImageElement mContent.
21) HTMLSelectElement::Add -- is working with one of the options.
22) nsTextEditorState::GetMaxLength -- has an <input> or <textarea>.

So all existing consumers are safe... I believe we can unhide this bug.

Comment 6

[Andrew McCreight [:mccr8]]

Marking old branches as unaffected. We should leave this hidden until it is fixed on Nightly.

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK, so the history of this code is....

We used to create an actual HTML element, with localName "img" and in the null namespace.

Then in bug 249809 we switched to creating an nsGenConImageContent, with localName "img" and in the null namespace.

Then in bug 238072 we started creating a thing with localName "img" and in the XHTML namespace, but still nsGenConImageContent.

Then in bug 451168 we switched the localName to "mozgeneratedcontentimage".

We should have changed the inheritance in bug 238072, I think.

Comment 8

[Andrew McCreight [:mccr8]]

Emilio points out bug 1434340, which is very similar or maybe a dupe.

Comment 11

[Calixte Denizet (:calixte)]

The signature 'nsContentUtils::ContentIsDraggable' is ranked #6 for content process in nightly top-crashers and it's marked as a startup crash.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Make nsGenConImageContent's inheritance match the way it's used

MozReview-Commit-ID: AYL4iZkMJiH

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 8946758 [details] [diff] [review]
Make nsGenConImageContent's inheritance match the way it's used

Review of attachment 8946758 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsContentCreatorFunctions.h
@@ +73,5 @@
>                   already_AddRefed<mozilla::dom::NodeInfo>&& aNodeInfo,
>                   mozilla::dom::FromParser aFromParser);
>  
> +already_AddRefed<nsIContent>
> +CreateGenConImageContent(nsIDocument* aDocument, imgRequestProxy* aImageRequest);

Shouldn't this either be in some mozilla:: namespace or have the NS_ prefix? Not that I think there's any risk of name collision.

::: dom/base/nsGenConImageContent.cpp
@@ +30,4 @@
>                                     public nsImageLoadingContent
>  {
>  public:
> +  explicit nsGenConImageContent(already_AddRefed<dom::NodeInfo>& aNodeInfo)

side note: It feels like the ctor should be private, and then you'd have a static Create() method which CreateGenConImageContent would call. That's a preexisting problem though, so fixing it doesn't seem like something for this bug.

@@ +35,5 @@
>    {
>      // nsImageLoadingContent starts out broken, so we start out
>      // suppressed to match it.
>      AddStatesSilently(NS_EVENT_STATE_SUPPRESSED);
> +    MOZ_ASSERT(IsHTMLElement(), "Someone messed up our nodeinfo");

Is it okay that nsGenericHTMLElement::IsHTMLElement() just always returns true? At that point, having a static assert that this class is a subclass of nsGenericHTMLElement feels stronger, though I suppose the semantic meaning of that check is more oblique. It feels like you should explicitly call nsIContent::IsHTMLElement() if you really want to check the nodeinfo.

@@ +98,5 @@
> +  // Work around not being able to bind a non-const lvalue reference
> +  // to an rvalue of non-reference type by just creating an rvalue
> +  // reference.
> +  already_AddRefed<dom::NodeInfo>&& rvalue = nodeInfo.forget();
> +  RefPtr<nsGenConImageContent> it = new nsGenConImageContent(lvalue);

Can you just make the ctor take a && to avoid this?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Shouldn't this either be in some mozilla:: namespace or have the NS_ prefix?

I can put it in mozilla::dom.

> Is it okay that nsGenericHTMLElement::IsHTMLElement() just always returns true?

Hmm.  Yes. I know I'm a subclass of nsGenericHTMLElement.  I'm trying to check the namespace here.

I'll just make this an explicit IsInNamespace() check.

> Can you just make the ctor take a && to avoid this?

No, because then the macro-generated Clone fails to compile...  I'll document.

Comment 15

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73c8f5ce12856fef30e2d411560d92520569d24a

Comment 16

[Calixte Denizet (:calixte)]

There few crashes with FennecAndroid and signature "kiss_fft": http://bit.ly/2DPZq4P
The function called before kiss_fft is nsFrame::HandlePress.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/73c8f5ce1285

Comment 19

[Calixte Denizet (:calixte)]

No more crashes in nightly 60 since the patch landed.