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] (largely AFK until June 28)
:
Mentors:
Depends on:
Blocks: 613899
  Show dependency treegraph
 
Reported: 2011-09-02 14:31 PDT by Daniel Holbert [:dholbert] (largely AFK until June 28)
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] (largely AFK until June 28)
roc: review+
Details | Diff | Review
testcase for GDB verification that GetOwnerDoc is what we want (806 bytes, patch)
2011-09-02 15:29 PDT, Daniel Holbert [:dholbert] (largely AFK until June 28)
no flags Details | Diff | Review

Description Daniel Holbert [:dholbert] (largely AFK until June 28) 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] (largely AFK until June 28) 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] (largely AFK until June 28) 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] (largely AFK until June 28) 2011-09-06 14:48:05 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/bca514585c99
Comment 4 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-07 07:56:51 PDT
http://hg.mozilla.org/mozilla-central/rev/bca514585c99

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