The default bug view has changed. See this FAQ.

Status

()

Core
Canvas: 2D
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: smaug, Assigned: roc)

Tracking

({regression, verified-aurora, verified-beta})

Trunk
mozilla11
regression, verified-aurora, verified-beta
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox8 affected, firefox9- fixed, firefox10- fixed, firefox11- fixed)

Details

(Whiteboard: [qa!])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 576805 [details]
testcase

I was told that this testcase crashes on Window 7 / Firefox 8.
Couldn't reproduce on Linux.
(Reporter)

Updated

5 years ago
Group: core-security
(Reporter)

Updated

5 years ago
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
(Reporter)

Comment 2

5 years ago
I don't know who owns canvas nowadays.
(Reporter)

Comment 3

5 years ago
To reproduce, download the testcase and run it locally.
It seems to cause OOM, but I don't understand why.
(Reporter)

Comment 4

5 years ago
I was told the crash happens on FF8 and Nightly
(Reporter)

Comment 5

5 years ago
And I was told the crash does not happen on FF7.
Keywords: regression, regressionwindow-wanted
WFM on Windows 7 trunk.
Created attachment 576820 [details]
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
Created attachment 576823 [details]
Simplified testcase

No image or imagedata stuff is needed.
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.
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.
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox8: --- → affected
status-firefox9: --- → affected
tracking-firefox10: --- → ?
tracking-firefox11: --- → ?
tracking-firefox9: --- → ?
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/74fed050ea42
Whiteboard: [inbound]
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
Last Resolved: 5 years ago
Flags: in-testsuite?
OS: Windows 7 → All
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla11
Version: unspecified → Trunk

Updated

5 years ago
Attachment #576829 - Flags: approval-mozilla-beta?
Attachment #576829 - Flags: approval-mozilla-beta+
Attachment #576829 - Flags: approval-mozilla-aurora?
Attachment #576829 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/898c412561c9
https://hg.mozilla.org/releases/mozilla-aurora/rev/52cd292898bd

https://hg.mozilla.org/releases/mozilla-beta/rev/05d765ddd56c
https://hg.mozilla.org/releases/mozilla-beta/rev/82f3d54c8fd9
status-firefox10: affected → fixed
status-firefox11: affected → fixed
status-firefox9: affected → fixed
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
Keywords: verified-aurora, verified-beta
Whiteboard: [qa+] → [qa!]

Updated

5 years ago
tracking-firefox10: ? → -
tracking-firefox11: ? → -
tracking-firefox9: ? → -
Keywords: regressionwindow-wanted
You need to log in before you can comment on or make changes to this bug.