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

RESOLVED FIXED in mozilla9

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
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.)
(Assignee)

Comment 1

6 years ago
Created attachment 557956 [details] [diff] [review]
fix

I tested this on all the testcases posted / linked off of bug 613899, as a sanity check.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #557956 - Flags: review?(roc)
(Assignee)

Comment 2

6 years ago
Created attachment 557964 [details] [diff] [review]
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)
Attachment #557956 - Flags: review?(roc) → review+
(Assignee)

Comment 3

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/bca514585c99
Whiteboard: [inbound]
(Assignee)

Updated

6 years ago
Target Milestone: --- → mozilla9
http://hg.mozilla.org/mozilla-central/rev/bca514585c99
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.