Last Comment Bug 665209 - Hang with recursive "content: url(data:image/svg+xml,...)"
: Hang with recursive "content: url(data:image/svg+xml,...)"
Status: VERIFIED FIXED
: crash, hang, mlk, regression, testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla7
Assigned To: Daniel Holbert [:dholbert] (largely AFK until June 28)
:
Mentors:
Depends on:
Blocks: randomstyles 308590
  Show dependency treegraph
 
Reported: 2011-06-17 15:48 PDT by Jesse Ruderman
Modified: 2013-12-27 14:28 PST (History)
7 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
fixed


Attachments
testcase (causes a hang after being closed) (317 bytes, text/html)
2011-06-17 15:48 PDT, Jesse Ruderman
no flags Details
stack trace (repeating portion only) (14.45 KB, text/plain)
2011-06-18 20:46 PDT, Jesse Ruderman
no flags Details
fix v1 (1.37 KB, patch)
2011-06-22 17:23 PDT, Daniel Holbert [:dholbert] (largely AFK until June 28)
no flags Details | Diff | Review
fix v1a (now with crashtest) (2.34 KB, patch)
2011-06-22 18:52 PDT, Daniel Holbert [:dholbert] (largely AFK until June 28)
no flags Details | Diff | Review
fix v2 (now in nsDataDocumentContentPolicy::ShouldLoad) (3.25 KB, patch)
2011-06-22 20:12 PDT, Daniel Holbert [:dholbert] (largely AFK until June 28)
bzbarsky: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Review

Description Jesse Ruderman 2011-06-17 15:48:28 PDT
Created attachment 540159 [details]
testcase (causes a hang after being closed)

1. Load the testcase.

2. Quit Firefox --> hang during shutdown
 or
2. Close testcase --> hang after a while (during CC or GC?)

It was difficult (impossible?) to construct such a testcase before the fix for bug 308590.

The attached version seems to hang consistently, but some variants sometimes leak nsDocument+ rather than hanging.  That's how my fuzzer found the bug: it's set to ignore hangs on pages that use SVG (due to bug 408147), but not leaks.
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-17 21:35:59 PDT
So the point is that we have an SVG-as-image which includes itself as an image using the relative URI '#'.  Normally SVG-as-image can't do resource loads, so the problem can't arise with http:// URIs, but we make an exception for "local resources" (which includes data:).

I believe allowing data: there is on purpose, but then we need recursion protection similar to what iframes do, right?
Comment 2 Jesse Ruderman 2011-06-18 20:45:29 PDT
On Windows, the hang is interrupted after a minute with a stack-exhaustion crash.
Comment 3 Jesse Ruderman 2011-06-18 20:46:10 PDT
Created attachment 540280 [details]
stack trace (repeating portion only)
Comment 4 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-06-22 17:23:26 PDT
Created attachment 541238 [details] [diff] [review]
fix v1

This appears to fix it for me.  Basically, we just check the image URI vs. its referrer, right before we load it, and don't load if they match (or if we're unable to determine if they match).
Comment 5 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-06-22 18:52:32 PDT
Created attachment 541256 [details] [diff] [review]
fix v1a (now with crashtest)

(added this bug's testcase as a crashtest.  Verified locally that a targeted crashtest run with it will fail (shutdown-hang) before the fix & pass after the fix.  I'm double-checking that on Tryserver, too.)
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-22 18:58:09 PDT
Comment on attachment 541238 [details] [diff] [review]
fix v1

This would affect all CSS image loads...  Can we restrict this to SVG-as-image?  e.g. by changing the data document content policy code.
Comment 7 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-06-22 20:12:58 PDT
Created attachment 541272 [details] [diff] [review]
fix v2 (now in nsDataDocumentContentPolicy::ShouldLoad)

Ah, good point.  This version moves the check to nsDataDocumentContentPolicy::ShouldLoad (which, incidentally, gets called just before the code in my previous patch, by way of nsContentUtils::CanLoadImage)
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-22 20:29:45 PDT
Comment on attachment 541272 [details] [diff] [review]
fix v2 (now in nsDataDocumentContentPolicy::ShouldLoad)

r=me
Comment 9 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-06-22 22:26:48 PDT
Landed on m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/bd7c4c011201
Comment 10 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-06-23 10:36:44 PDT
http://hg.mozilla.org/mozilla-central/rev/bd7c4c011201
Comment 11 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-06-23 10:48:40 PDT
Comment on attachment 541272 [details] [diff] [review]
fix v2 (now in nsDataDocumentContentPolicy::ShouldLoad)

Requesting approval to land on Aurora (for Firefox 6).

Benefit:
 - Fixes hang
 - Includes test

Risk assessment: Low.
 - Only affects SVG-as-an-image (code touched is inside of an "if doc->IsBeingUsedAsImage()" clause)
 - Just adds an additional "can I load this content" rejection case. (when an image to-be-loaded matches our document's own URI)
Comment 12 Asa Dotzler [:asa] 2011-06-23 14:33:11 PDT
Comment on attachment 541272 [details] [diff] [review]
fix v2 (now in nsDataDocumentContentPolicy::ShouldLoad)

Approved to land on Aurora. Please land soon. Thanks.
Comment 13 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-06-23 15:04:07 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/0db25764b0f7
Comment 14 Virgil Dicu [:virgil] [QA] 2011-08-26 04:19:40 PDT
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux i686; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0

Verified fixed in F7 beta2, using the test case provided in comment 0. Checked on Windows XP, Windows 7, Ubuntu 11.04, Mac OS 10.6.

Note You need to log in before you can comment on or make changes to this bug.