If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Init method of nsIDocumentEncoder does not reset member variables

VERIFIED FIXED

Status

()

Core
Serializers
--
major
VERIFIED FIXED
13 years ago
13 years ago

People

(Reporter: David Gardiner, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

13 years ago
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.
(Reporter)

Updated

13 years ago
Blocks: 278438
(Reporter)

Comment 1

13 years ago
Created attachment 171505 [details] [diff] [review]
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
(Reporter)

Updated

13 years ago
Attachment #171505 - Flags: review?(dean_tessman)

Comment 2

13 years ago
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
Attachment #171505 - Flags: review?(dean_tessman) → review+
(Reporter)

Comment 3

13 years ago
(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
(Reporter)

Comment 4

13 years ago
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
Attachment #171505 - Flags: superreview?(neil.parkwaycc.co.uk)
(Reporter)

Comment 5

13 years ago
Should I set any of the blocking flags on this?

-dave

Comment 6

13 years ago
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.
Attachment #171505 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-

Comment 7

13 years ago
Sorry, I mean to cancel that request, not deny it :-[
(Reporter)

Updated

13 years ago
Attachment #171505 - Flags: superreview- → superreview?(bzbarsky)
(Reporter)

Updated

13 years ago
Attachment #171505 - Flags: superreview?(bzbarsky)
(Reporter)

Updated

13 years ago
Attachment #171505 - Flags: superreview?(jst)
Comment on attachment 171505 [details] [diff] [review]
Reset member variables when Init method is called

sr=jst
Attachment #171505 - Flags: superreview?(jst) → superreview+

Comment 9

13 years ago
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
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.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
*** Bug 278438 has been marked as a duplicate of this bug. ***
Now verifying.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.