Closed Bug 705173 Opened 8 years ago Closed 8 years ago

Crash with canvas

Categories

(Core :: Canvas: 2D, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla11
Tracking Status
firefox8 --- affected
firefox9 - fixed
firefox10 - fixed
firefox11 - fixed

People

(Reporter: smaug, Assigned: roc)

Details

(Keywords: regression, verified-aurora, verified-beta, Whiteboard: [qa!])

Attachments

(3 files, 1 obsolete file)

Attached file testcase (obsolete) —
I was told that this testcase crashes on Window 7 / Firefox 8.
Couldn't reproduce on Linux.
Group: core-security
Group: core-security
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I don't know who owns canvas nowadays.
To reproduce, download the testcase and run it locally.
It seems to cause OOM, but I don't understand why.
I was told the crash happens on FF8 and Nightly
And I was told the crash does not happen on FF7.
WFM on Windows 7 trunk.
Attached file working testcase
This testcase should work better. It makes the image a data: URI so the canvas is origin-clean.
Assignee: nobody → roc
Attachment #576805 - Attachment is obsolete: true
Attached file Simplified testcase
No image or imagedata stuff is needed.
Attached patch fixSplinter Review
Works on this testcase at least.

Not sure how to write an automated test for the memory usage.
Attachment #576829 - Flags: review?(bas.schouten)
This is an Azure regression. We can probably live without fixing it on 8, but we should fix it in aurora and beta.
(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 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 :).
Attachment #576829 - Flags: review?(bas.schouten) → review+
(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 on attachment 576829 [details] [diff] [review]
fix

We need to fix this regression everywhere we can.
Attachment #576829 - Flags: approval-mozilla-beta?
Attachment #576829 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/74fed050ea42
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
OS: Windows 7 → All
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla11
Version: unspecified → Trunk
Attachment #576829 - Flags: approval-mozilla-beta?
Attachment #576829 - Flags: approval-mozilla-beta+
Attachment #576829 - Flags: approval-mozilla-aurora?
Attachment #576829 - Flags: approval-mozilla-aurora+
Whiteboard: [qa+]
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.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.