1192654 - "Save As" crashes on cnn.com (and anything else with iframes from nonpersistable URI schemes)

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Simon Lindholm]

1. Visit http://edition.cnn.com/.
2. Press Ctrl+S, choose some file name, press enter.
-> Firefox crashes from a null pointer dereference.

https://crash-stats.mozilla.com/report/index/2d5f445b-a70e-4b63-a8ac-5b3882150809

mozregress gives https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2fad09a0540d&tochange=91de9c670800 which contains bug 1101100 as an obvious suspect.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Here's a more self-contained example: data:text/html,<iframe%20src="data:text/plain,Example">

I was *sure* I tested this case....

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Patch: restore null pointer check

The fix is tiny, but I also added a test case (adapted from the one for bug 1170334) and checked that it reproduces the crash without this fix.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8646554 [details] [diff] [review]
Patch: restore null pointer check

Review of attachment 8646554 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the test.

::: embedding/test/test_bug1192654.html
@@ +59,5 @@
> +    }
> +  };
> +  wbp.saveDocument(doc, tmpFile, tmpDir, null, 0, 0);
> +
> +  function cleanUp() {

Maybe use registerCleanupFunction for this just in case the test fails?

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Patch: restore null pointer check [v1.1]

Thanks for the review.  Updated patch; carrying over r+.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Checkin needed.  https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1b34194f4cf is from before I adjusted the test (comment #3), but I retested it locally.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/c6b9b5f5c6c6

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8647007 [details] [diff] [review]
Patch: restore null pointer check [v1.1]

Approval Request Comment
[Feature/regressing bug #]: Bug 1101100
[User impact if declined]: Crashes when using Save As on popular websites (see comment #0)
[Describe test coverage new/current, TreeHerder]: This patch adds a regression test, and there are some existing tests for nsWebBrowserPersist.
[Risks and why]: Basically none; this patch won't do anything except in the case that was previously crashing.
[String/UUID change made/needed]: None.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c6b9b5f5c6c6

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8647007 [details] [diff] [review]
Patch: restore null pointer check [v1.1]

Fix a crash, taking it.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/20ef2dbd579e