317 bytes, text/html
14.45 KB, text/plain
3.25 KB, patch
|Details | Diff | Splinter Review|
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.
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?
On Windows, the hang is interrupted after a minute with a stack-exhaustion crash.
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).
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 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.
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 on attachment 541272 [details] [diff] [review] fix v2 (now in nsDataDocumentContentPolicy::ShouldLoad) r=me
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 on attachment 541272 [details] [diff] [review] fix v2 (now in nsDataDocumentContentPolicy::ShouldLoad) Approved to land on Aurora. Please land soon. Thanks.
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.