Last Comment Bug 684358 - nsSVGFEImageElement calls inherited method nsImageLoadingContent::GetOurDocument when it could directly call nsINode::GetOwnerDoc
: nsSVGFEImageElement calls inherited method nsImageLoadingContent::GetOurDocum...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Daniel Holbert [:dholbert]
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 613899
  Show dependency treegraph
 
Reported: 2011-09-02 14:31 PDT by Daniel Holbert [:dholbert]
Modified: 2011-09-07 07:56 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (1.10 KB, patch)
2011-09-02 15:07 PDT, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review
testcase for GDB verification that GetOwnerDoc is what we want (806 bytes, patch)
2011-09-02 15:29 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2011-09-02 14:31:13 PDT
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] 2011-09-02 15:07:28 PDT
Created attachment 557956 [details] [diff] [review]
fix

I tested this on all the testcases posted / linked off of bug 613899, as a sanity check.
Comment 2 Daniel Holbert [:dholbert] 2011-09-02 15:29:27 PDT
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)
Comment 3 Daniel Holbert [:dholbert] 2011-09-06 14:48:05 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/bca514585c99

Note You need to log in before you can comment on or make changes to this bug.