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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[timeless]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[timeless]

Attachment: proposal per bz's comment

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 496383 [details] [diff] [review]
proposal per bz's comment

Please add curlies around the if body.

Comment 4

[timeless]

Attachment: add curlies!

bz: do you really want this instead?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 496465 [details] [diff] [review]
add curlies!

Sure, but you already had r+...  Why ask again?

Comment 6

[timeless]

because it was changing other parts of the function. i wanted to make sure you really wanted the full change.

Comment 7

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/2c7df2c32e3a