Closed Bug 1434273 Opened 6 years ago Closed 6 years ago

Crash in nsContentUtils::ContentIsDraggable

Categories

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

Unspecified
Windows 10
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- verified

People

(Reporter: calixte, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(5 keywords)

Crash Data

Attachments

(1 file)

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
Flags: needinfo?(bzbarsky)
Crash Signature: [@ nsContentUtils::ContentIsDraggable] → [@ nsContentUtils::ContentIsDraggable] [@ nsFrame::HandlePress]
See Also: → 1434315
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.... :(
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.
Assignee: nobody → bzbarsky
Group: dom-core-security, core-security
Group: core-security
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.
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.
Marking old branches as unaffected. We should leave this hidden until it is fixed on Nightly.
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.
Emilio points out bug 1434340, which is very similar or maybe a dupe.
Crash Signature: [@ nsContentUtils::ContentIsDraggable] [@ nsFrame::HandlePress] → [@ nsContentUtils::ContentIsDraggable] [@ nsFrame::HandlePress] [@ mozilla::a11y::Accessible::NativeAttributes] [@ nsFrame::HandleEvent]
The signature 'nsContentUtils::ContentIsDraggable' is ranked #6 for content process in nightly top-crashers and it's marked as a startup crash.
Keywords: topcrash
MozReview-Commit-ID: AYL4iZkMJiH
Attachment #8946758 - Flags: review?(continuation)
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?
Attachment #8946758 - Flags: review?(continuation) → review+
> 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.
Flags: needinfo?(bzbarsky)
There few crashes with FennecAndroid and signature "kiss_fft": http://bit.ly/2DPZq4P
The function called before kiss_fft is nsFrame::HandlePress.
Crash Signature: [@ nsContentUtils::ContentIsDraggable] [@ nsFrame::HandlePress] [@ mozilla::a11y::Accessible::NativeAttributes] [@ nsFrame::HandleEvent] → [@ nsContentUtils::ContentIsDraggable] [@ nsFrame::HandlePress] [@ mozilla::a11y::Accessible::NativeAttributes] [@ nsFrame::HandleEvent] [@ kiss_fft]
https://hg.mozilla.org/mozilla-central/rev/73c8f5ce1285
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
No more crashes in nightly 60 since the patch landed.
Status: RESOLVED → VERIFIED
Group: dom-core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: