459424 - [FIX]Null-deref crash when zooming on page with invalid external resource documents

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We pass null documents to the enumerator in that situation.  Fix coming up.  Jesse, do we have a good way of crashtesting something like this?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 2

[Boris Zbarsky [:bzbarsky]]

Jesse, see comment 0?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Test page

Comment 4

[Jesse Ruderman]

There are a few existing automated tests that mess with zoom, such as these tests that use nsIMarkupDocumentViewer.fullZoom:

http://mxr.mozilla.org/mozilla-central/search?string=fullzoom&find=test

I think you're supposed to use a mochitest to ensure privileges are available.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 342625 [details] [diff] [review]
Fix

+  PRBool next =
+    aData->mDocument ? args->callback(aData->mDocument, args->data) : PR_TRUE;

aData->mDocument && args->callback(...)

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm going to add zoom support to reftests in bug 458487 (or maybe in a subordinate bug spun off from that).

Comment 7

[Boris Zbarsky [:bzbarsky]]

> aData->mDocument && args->callback(...)

You mean:

  !aData->mDocument || args->callback(...);

?  I can do that I guess; but do you really think it's more readable?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Pushed changeset b5b7b5523c66 with test.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This was backed out as it seemed the most likely cause of multiple unit test failures.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The backout did, in fact, clear up the unit test failures.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Checked back in, with a change to the test to reset zoom back to 1 to fix the failures.