278702 - Init method of nsIDocumentEncoder does not reset member variables

Status: VERIFIED FIXED

(Core :: DOM: Serializers, defect)

Description

[David Gardiner]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050116

If (as I had assumed) calling the Init method of classes inheriting from
nsIDocumentEncoder (eg. nsHTMLCopyEncoder) a second time to reuse an existing
object, you experience a bug caused by some of the member variables only being
initialised inside the constructor.

I believe this is the root cause of bug 278438 

Reproducible: Always

Actual Results:  
Calling Init(..) a second time on an existing nsIDocumentEncoder object then
calling another method eg. SetSelection or EncodeToStringWithContext returns
unexpected results

Expected Results:  
Calling Init(..) should reset all internal member variables so that the object
can be reused without worrying about previous uses.

Comment 1

[David Gardiner]

Attachment: Reset member variables when Init method is called

Dean,
this fixes a problem that has occurred after my SourceURL Copy/Paste bug 244685
patch landed.

I have a workaround patch in bug 278438 for the code that experiences the
problem, but this patch would actually solve the problem properly.

thanks,
-dave

Comment 2

[:deanis74]

Comment on attachment 171505 [details] [diff] [review]
Reset member variables when Init method is called

This looks fairly straight-forward to me.

>@@ -1064,19 +1073,23 @@ nsHTMLCopyEncoder::~nsHTMLCopyEncoder()
> NS_IMETHODIMP
> nsHTMLCopyEncoder::Init(nsIDocument* aDocument,
>                         const nsAString& aMimetype,
>                         PRUint32 aFlags)
> {
>   if (!aDocument)
>     return NS_ERROR_INVALID_ARG;
> 
>+  mIsTextWidget = PR_FALSE;
>+  Initialize();

Can't mIsTextWidget be set to PR_FALSE in the base Initialize()?  Any place
that sets it to PR_TRUE is called directly.

Also, does this negate the change that was made for bug 273741, or is that
needed regardless?

r=me with those questions answered

Comment 3

[David Gardiner]

(In reply to comment #2)
> Can't mIsTextWidget be set to PR_FALSE in the base Initialize()?  Any place
> that sets it to PR_TRUE is called directly.

mIsTextWidget is only declared in the nsHTMLCopyEncoder (not in
nsDocumentEncoder where i put the Initialize method). I guess I could add an
override method in the subclass - what do you think?
 
> Also, does this negate the change that was made for bug 273741, or is that
> needed regardless?

I think that change is still needed - there was a flaw in how I handled the
plaintext vs. HTML format stuff that this patch wouldn't solve.

-dave

Comment 4

[David Gardiner]

Comment on attachment 171505 [details] [diff] [review]
Reset member variables when Init method is called

Neil,
this fixes a problem has appeared after my SourceURL Copy/Paste bug 244685
patch landed.

The Init() method didn't reset everything when it was called a second (or more)
times.

-dave

Comment 5

[David Gardiner]

Should I set any of the blocking flags on this?

-dave

Comment 6

[[e-mail]]

Comment on attachment 171505 [details] [diff] [review]
Reset member variables when Init method is called

Sorry, not my area, please check the list at
http://www.mozilla.org/hacking/reviewers.html for a more suitable victim.

Comment 7

[[e-mail]]

Sorry, I mean to cancel that request, not deny it :-[

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 171505 [details] [diff] [review]
Reset member variables when Init method is called

sr=jst

Comment 9

[Ian Neal]

Comment on attachment 171505 [details] [diff] [review]
Reset member variables when Init method is called

Checking in nsDocumentEncoder.cpp;
/cvsroot/mozilla/content/base/src/nsDocumentEncoder.cpp,v  <-- 
nsDocumentEncoder.cpp
new revision: 1.104; previous revision: 1.103
done

Comment 10

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED with the 2005-01-21-05 Windows XP Seamonkey trunk builds using my
testcase in bug 278438.  I'll go ahead and just mark that bug a straight DUP of
this.

Thanks, David, and sorry for jumping the gun on the patch commit yesterday.

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 278438 has been marked as a duplicate of this bug. ***

Comment 12

[Stephen Donner [:stephend] Not actively reading bugmail]

Now verifying.