684358 - nsSVGFEImageElement calls inherited method nsImageLoadingContent::GetOurDocument when it could directly call nsINode::GetOwnerDoc

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Just noticed that nsSVGFEImageElement makes a call to its inherited method "GetOurDocument" that probably should be "GetOwnerDoc".

The code in question, which I added in bug 613899:

> 5425 nsSVGFEImageElement::LoadSVGImage(PRBool aForce, PRBool aNotify)
> 5426 {
[...]
> 5437   // Make sure we don't get in a recursive death-spiral
> 5438   nsIDocument* doc = GetOurDocument();
https://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGFilters.cpp#5425

GetOurDocument is just a wrapper for nsINode::GetOwnerDoc:

> 862 nsIDocument*
> 863 nsImageLoadingContent::GetOurDocument()
> 864 {
> 865   nsCOMPtr<nsIContent> thisContent = do_QueryInterface(this);
> 866   NS_ENSURE_TRUE(thisContent, nsnull);
> 867 
> 868   return thisContent->GetOwnerDoc();
> 869 }
https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsImageLoadingContent.cpp#863

We know we inherit from nsIContent/nsINode, so we don't need the overhead of the QI, and would be better of skipping it and just directly calling GetOwnerDoc.

(We might be able to get away with GetCurrentDoc(), but I'm not absolutely sure IsInDoc() will be true every time that LoadSVGImage() is triggered, so GetOwnerDoc() is safer.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix

I tested this on all the testcases posted / linked off of bug 613899, as a sanity check.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase for GDB verification that GetOwnerDoc is what we want

(In reply to Daniel Holbert [:dholbert] from comment #0)
> (We might be able to get away with GetCurrentDoc(), but I'm not absolutely
> sure IsInDoc() will be true every time that LoadSVGImage() is triggered, so
> GetOwnerDoc() is safer.)

Confirmed in GDB that we do really want GetOwnerDoc() here, with this testcase.  Document-load triggers the feImage creation & xlink:href setting, which triggers LoadSVGImage -- but GetCurrentDoc() is null at that point, since we're not attached to the document's tree yet.  (we stay unattached until the onclick handler fires)

Comment 3

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/bca514585c99

Comment 4

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/bca514585c99