FMR in nsParser::Terminate

VERIFIED FIXED

Status

()

Core
HTML: Parser
P3
critical
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: John Bandhauer, Assigned: harishd)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm++][Fix in hand] jband, please verify)

Attachments

(1 attachment)

(Reporter)

Description

18 years ago
You return mInternalState even after calling NS_RELEASE(me). This has the 
potential to read from a member of an object that is already deleted (I'm 
seeing that happen in Purify). Should "return result"

nsresult nsParser::Terminate(void){
  nsresult result=NS_OK;
  if(mParserContext && mParserContext->mDTD) {
    result=mParserContext->mDTD->Terminate(this);
    if(result==NS_ERROR_HTMLPARSER_STOPPARSING) {
      // XXX - [ until we figure out a way to break parser-sink circularity ]
      // Hack - Hold a reference until we are completely done...
      nsIParser* me=(nsIParser*)this;  // <-- this should be a static cast
      NS_ADDREF(me); 
      mInternalState=result;
      result=DidBuildModel(result);
      NS_RELEASE(me);
      return result;    //  <---- new line
    }
  }
  return mInternalState;
}

Also note that you should not use a plain C cast to get at the interface part of 
an object. This does the wrong thing if the interface is not the left-most.

Let's fix this fast.
(Assignee)

Comment 1

18 years ago
Will do it. Thanx
Status: NEW → ASSIGNED
(Assignee)

Comment 2

18 years ago
Marking for rtm.
Keywords: rtm

Comment 3

18 years ago
Marking rtm+.  Recommending to PDT to rtm+- this bug because it isn't high
priority enough for the rtm branch.
Whiteboard: [rtm+][Fix in hand]
(Reporter)

Comment 4

18 years ago
Reading free'd memory is a potential crasher. Seems like a damn fine thing to 
fix to me. Please attach the "fix in hand". I expect it to have near zero risk.

Comment 5

18 years ago
PDT marking [rtm need info] because this patch needs code reviews. If no crash
can be demonstrated, this bug is going to be [rtm-] by the end of the week.
Whiteboard: [rtm+][Fix in hand] → [rtm need info][Fix in hand]
(Assignee)

Comment 6

18 years ago
r=me ( patch provided by jband ) will get a= shortly.
(Assignee)

Comment 7

18 years ago
Created attachment 16143 [details] [diff] [review]
Patch [ derived from jband's patch ] r=jst,me

Comment 8

18 years ago
a=vidur. This seems safe enough for now, but post rtm we need to rethink the 
ownership model for the parser, sink and document. I think we're substituting 
the kungFuDeathGrip trick for a coherent understanding of the right order of 
destruction.

Comment 9

18 years ago
Marking rtm+ to get back onto the PDT radar.  The patch has been approved by
vidur and reviewed by jst/harishd.
Whiteboard: [rtm need info][Fix in hand] → [rtm+][Fix in hand]

Comment 10

18 years ago
PDT marking [rtm++]
Whiteboard: [rtm+][Fix in hand] → [rtm++][Fix in hand]
(Assignee)

Comment 11

18 years ago
Fix is in the branch & trunk.. Marking FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 12

18 years ago
jband, please verify
Whiteboard: [rtm++][Fix in hand] → [rtm++][Fix in hand] jband, please verify
(Reporter)

Comment 13

18 years ago
looks good to me.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.