Closed Bug 602516 Opened 9 years ago Closed 9 years ago

Either PresShell::Freeze needs to consistently null check mDocument to avoid a crash [@ PresShell::UpdateImageLockingState]

Categories

(Core :: Layout, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: timeless, Assigned: timeless)

References

(Blocks 1 open bug)

Details

(Keywords: coverity)

Attachments

(1 file, 1 obsolete file)

7273 	PresShell::Freeze()

deref mDocument:
7277 	  mDocument->EnumerateFreezableElements(FreezeElement, nsnull);

7279 	  if (mCaret)
7280 	    mCaret->SetCaretVisible(PR_FALSE);

Null check for mDocument:
7284 	  if (mDocument)
7285 	    mDocument->EnumerateSubDocuments(FreezeSubDocument, nsnull);

7287 	  nsPresContext* presContext = GetPresContext();
7288 	  if (presContext &&
7289 	      presContext->RefreshDriver()->PresContext() == presContext) {
7290 	    presContext->RefreshDriver()->Freeze();
7291 	  }
7292 	
7293 	  mFrozen = PR_TRUE;

7294 	  UpdateImageLockingState();

9009 	PresShell::UpdateImageLockingState()
9010 	{
9011 	  // We're locked if we're both thawed and active.
9012 	  return mDocument->SetImageLockingState(!mFrozen && mIsActive);

I think it's reasonable to assert that if mDocument were actually null, we'd have noticed crashes by now and that the null check is useless.
It really depends on the exact plugins running.  I believe FreezeElement can tear down the world if the plugin is not well-behaved, so we do need a null-check in UpdateImageLockingState.
Summary: Either PresShell::Freeze uselessly null checks mDocument or crash [@ PresShell::UpdateImageLockingState] → Either PresShell::Freeze needs to consistently null check mDocument to avoid a crash [@ PresShell::UpdateImageLockingState]
Attached patch proposal per bz's comment (obsolete) — Splinter Review
Attachment #496383 - Flags: review?(bzbarsky)
Attachment #496383 - Flags: approval2.0?
Comment on attachment 496383 [details] [diff] [review]
proposal per bz's comment

Please add curlies around the if body.
Attachment #496383 - Flags: review?(bzbarsky)
Attachment #496383 - Flags: review+
Attachment #496383 - Flags: approval2.0?
Attachment #496383 - Flags: approval2.0+
Attached patch add curlies!Splinter Review
bz: do you really want this instead?
Attachment #496465 - Flags: review?(bzbarsky)
Attachment #496465 - Flags: approval2.0?
Comment on attachment 496465 [details] [diff] [review]
add curlies!

Sure, but you already had r+...  Why ask again?
Attachment #496465 - Flags: review?(bzbarsky)
Attachment #496465 - Flags: review+
Attachment #496465 - Flags: approval2.0?
Attachment #496465 - Flags: approval2.0+
because it was changing other parts of the function. i wanted to make sure you really wanted the full change.
Keywords: checkin-needed
Attachment #496383 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/2c7df2c32e3a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.