236812 - When getting IHtmlElement::parentElement property the control crashes

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: ActiveX Wrapper, defect)

Description

[Alexandre Trémon]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; fr-FR; rv:1.7b) Gecko/20040303
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; fr-FR; rv:1.7b) Gecko/20040303

In CIEHtmlElement::get_parentElement(), IDispatch *pDisp =
reinterpret_cast<IDispatch *>(mParent);
pDisp->QueryInterface(IID_IHTMLElement, (void **) p);
results in a crash.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Alexandre Trémon]

Attachment: Replace the reinterpret_cast to IDispatch by a static_cast to CIEHtmlElement

Reinterpret_cast doesn't move the pointer, so a static_cast should be
necessary. But the compiler won't allow this because it doesn't know that the
IDispatch and CIEHtmlNode are related. The solution is simply to cast to a
CIEHtmlElement

Comment 2

[Alexandre Trémon]

I'm not sure this does necessarily result in a crash, but might sometimes only
return a void pointer.

Steps to Reproduce: with #import <mshtml.tlb>

MSHTML::IHTMLElementPtr pEl = m_pHtmlDoc2->body;
MSHTML::IHTMLElementCollectionPtr pColl = pEl->children;
long pL;
pColl->get_length(&pL);
TRACE1("%d\n", pL);
COleVariant oleVar;
oleVar.vt = VT_I2;
oleVar.iVal = 0;
MSHTML::IHTMLElementPtr pEl2 = pColl->item(oleVar);
TRACE(pEl2->tagName);
MSHTML::IHTMLElementPtr pEl3 = pEl2->parentElement;

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 143289 [details] [diff] [review]
Replace the reinterpret_cast to IDispatch by a static_cast to CIEHtmlElement

There's no way that mParent node is *not* an element (CIEHtmlElement) object
here, right?

If so, sr=jst

Comment 4

[Alexandre Trémon]

Well, actually, mParent is changed by PopulateFromDOMHTMLCollection when
get_images, get_applets, get_links, etc. is called on IHTMLDocument, which for
me is wrong (eg. images are not a direct child of the document).
So I would remove the SetParent call from PopulateFromDOMHTMLCollection.

The same happens from CreateFromParentNode, either because of a call to get_all
in IHTMLDocument or IHTMLElement, or because of a call to get_children.
I would only leave the SetParent call in this last case, (and then mParent is
*always* a CIEHtmlElement), and remove it for a call to get_all (get_all returns
elements that are not necessarily a direct child of the element or document we
make the call on).

I could update the patch for bug
http://bugzilla.mozilla.org/show_bug.cgi?id=236894, explaining this, before
checking in this one.

Comment 5

[Adam Lock]

The parentElement can definitely be null if the element is the topmost in the
document, but I think the cast inside the pointer check should be fine. The
parent of an element should be another element, implemented by CIEHtmlElement.

Comment 6

[Adam Lock]

Fix is checked in