Last Comment Bug 705173 - Crash with canvas
: Crash with canvas
Status: VERIFIED FIXED
[qa!]
: regression, verified-aurora, verified-beta
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-24 11:19 PST by Olli Pettay [:smaug]
Modified: 2015-10-07 18:51 PDT (History)
11 users (show)
mounir: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
-
fixed
-
fixed
-
fixed


Attachments
testcase (1.20 KB, text/html)
2011-11-24 11:19 PST, Olli Pettay [:smaug]
no flags Details
working testcase (11.06 KB, text/html)
2011-11-24 12:52 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
Simplified testcase (489 bytes, text/html)
2011-11-24 13:22 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
fix (6.13 KB, patch)
2011-11-24 14:47 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
bas: review+
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Review

Description Olli Pettay [:smaug] 2011-11-24 11:19:50 PST
Created attachment 576805 [details]
testcase

I was told that this testcase crashes on Window 7 / Firefox 8.
Couldn't reproduce on Linux.
Comment 1 Ryan VanderMeulen [:RyanVM] 2011-11-24 11:32:25 PST
Confirmed on Windows 7 x64 with a mozilla-central build. The testcase loaded fine at first, then after a few seconds began hanging the browser.
Comment 2 Olli Pettay [:smaug] 2011-11-24 12:25:35 PST
I don't know who owns canvas nowadays.
Comment 3 Olli Pettay [:smaug] 2011-11-24 12:26:43 PST
To reproduce, download the testcase and run it locally.
It seems to cause OOM, but I don't understand why.
Comment 4 Olli Pettay [:smaug] 2011-11-24 12:28:32 PST
I was told the crash happens on FF8 and Nightly
Comment 5 Olli Pettay [:smaug] 2011-11-24 12:37:26 PST
And I was told the crash does not happen on FF7.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-24 12:43:18 PST
WFM on Windows 7 trunk.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-24 12:52:49 PST
Created attachment 576820 [details]
working testcase

This testcase should work better. It makes the image a data: URI so the canvas is origin-clean.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-24 13:22:17 PST
Created attachment 576823 [details]
Simplified testcase

No image or imagedata stuff is needed.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-24 14:47:16 PST
Created attachment 576829 [details] [diff] [review]
fix

Works on this testcase at least.

Not sure how to write an automated test for the memory usage.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-24 14:51:09 PST
This is an Azure regression. We can probably live without fixing it on 8, but we should fix it in aurora and beta.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-24 14:53:13 PST
(This bug is caused by the tmp_canvas's mDependentTargets keeping DrawTargets alive. tmp_canvas is never dirtied after the first draw so it will keep everything it draws to alive indefinitely.)
Comment 12 Bas Schouten (:bas.schouten) 2011-11-24 16:23:13 PST
Comment on attachment 576829 [details] [diff] [review]
fix

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

Looks good to me, with some changes. We should keep an eye on perf if this makes any difference.

::: gfx/2d/DrawTargetD2D.cpp
@@ +189,5 @@
>      mTempRT->EndDraw();
>    }
> +
> +  // Targets depending on us can break that dependency, since we're obviously not going to
> +  // be modified in the future.

Maybe this can pre made a little prettier by a local typedef std::vector<DrawTargetD2D*>::iterator target_iter or something of the likes.

@@ +200,5 @@
> +  for (std::vector<DrawTargetD2D*>::iterator iter = mDependingOnTargets.begin();
> +       iter != mDependingOnTargets.end(); iter++) {
> +    (*iter)->mDependentTargets.erase(
> +	  std::remove((*iter)->mDependentTargets.begin(), (*iter)->mDependentTargets.end(), this));
> +  }

We should use find() as discussed per IRC.

@@ +1287,4 @@
>        (*iter)->Flush();
>      }
> +	// The Flush() should have broken all dependencies on this target.
> +	MOZ_ASSERT(!mDependentTargets.size());

Somehow a tab ended up here :).
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-24 18:11:01 PST
(In reply to Bas Schouten (:bas) from comment #12)
> Looks good to me, with some changes. We should keep an eye on perf if this
> makes any difference.

I think this can only help perf. It can only do less flushing. Unless mDependentTargets/mDependingOnTargets get very large (in which case we already had a performance problem).

> Maybe this can pre made a little prettier by a local typedef
> std::vector<DrawTargetD2D*>::iterator target_iter or something of the likes.

target_iter isn't descriptive enough, this is more specific than that. Let's just revel in the pain of std::vector.

> We should use find() as discussed per IRC.

Done.

> @@ +1287,4 @@
> >        (*iter)->Flush();
> >      }
> > +	// The Flush() should have broken all dependencies on this target.
> > +	MOZ_ASSERT(!mDependentTargets.size());
> 
> Somehow a tab ended up here :).

Fixed.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-24 18:20:01 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/74fed050ea42
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-24 18:20:23 PST
Comment on attachment 576829 [details] [diff] [review]
fix

We need to fix this regression everywhere we can.
Comment 16 Mounir Lamouri (:mounir) 2011-11-25 02:28:37 PST
https://hg.mozilla.org/mozilla-central/rev/74fed050ea42
Comment 18 Virgil Dicu [:virgil] [QA] 2011-12-02 05:26:33 PST
Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0 Firefox Beta4
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a2) Gecko/20111201 Firefox/10.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:11.0a1) Gecko/20111202 Firefox/11.0a1
Mozilla/5.0 (Windows NT 5.1; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 5.1; rv:11.0a1) Gecko/20111201 Firefox/11.0a1
Mozilla/5.0 (Windows NT 5.1; rv:10.0a2) Gecko/20111201 Firefox/10.0a2
Mozilla/5.0 (Windows NT 6.1; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 6.1; rv:10.0a2) Gecko/20111201 Firefox/10.0a2
Mozilla/5.0 (Windows NT 6.1; rv:11.0a1) Gecko/20111201 Firefox/11.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0) Gecko/20100101
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0a2) Gecko/20111130 Firefox/10.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0a1) Gecko/20111130 Firefox/11.0a1

Verified using test case from comment 8 on Windows 7, XP, Ubuntu 11.10 and Mac OS 10.6. With Firefox9Beta4, Aurora and Nightly.

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