Closed Bug 1033310 Opened 5 years ago Closed 5 years ago

"ASSERTION: Failded to create DataSourceSurface" with huge canvas

Categories

(Core :: Canvas: 2D, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: jruderman, Assigned: milan)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

Attached file testcase
###!!! ASSERTION: Failded to create DataSourceSurface: 'Error', file content/canvas/src/ImageEncoder.cpp, line 302

(same assertion as in bug 1028522)
Attached file stack
This should just get cleaned up.  CreateDataSourceSurfaceWithStride() can very easily return nullptr, and we rarely check in the calling code. Same with CreateDataSourceSurface().
Assignee: nobody → milan
Right - can't use all the NS_* checks and warnings inside of gfx/2d.  https://tbpl.mozilla.org/?tree=Try&rev=dea9642c4377
Comment on attachment 8478515 [details] [diff] [review]
Handle CreateDataSourceSurface* returning nullptr and warn a lot

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

Nice to see some more clarification around potentially null returned values! Although I am not found of NS_ENSURE_TRUE since I am used to scanning for "return" when trying to understand the control flow. Btw, NS_ENSURE_TRUE is deprecated according to nsDebug.h (I suppose for the same reason, since it advocates the pattern "if(NS_WARN_IF(cond)) { return something; }" instead).
Attachment #8478515 - Flags: feedback?(nical.bugzilla) → feedback+
Didn't see the "it's preferred to use NS_WARN_IF", as it's above the definition of the macro; no point in propagating or introducing a pattern we don't like.  I'll make a change.
:nical looked at this and suggested the most recent change, but review for Bas as I want to double check the changes in Logging.h and RecordedEvent.cpp in particular.
Attachment #8479361 - Flags: review?(bas)
Attachment #8479361 - Attachment description: Handle CreateDataSourceSurface* returning nullptr and warn a lot → Handle CreateDataSourceSurface* returning nullptr and warn a lot using NS_WARN_IF
Comment on attachment 8479361 [details] [diff] [review]
Handle CreateDataSourceSurface* returning nullptr and warn a lot using NS_WARN_IF

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

Looks great.
Attachment #8479361 - Flags: review?(bas) → review+
Cool, but I would rather have this assert in the ShSurf code. I'll do a follow-up patch.
https://hg.mozilla.org/mozilla-central/rev/08cfde6dd148
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(In reply to Jeff Gilbert [:jgilbert] from comment #11)
> Cool, but I would rather have this assert in the ShSurf code. I'll do a
> follow-up patch.

You mean in the places that call (directly or indirectly) ImageEncoder::ExtractDataInternal?  Who should be asserting what?
Comment on attachment 8479361 [details] [diff] [review]
Handle CreateDataSourceSurface* returning nullptr and warn a lot using NS_WARN_IF

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

::: gfx/gl/SharedSurfaceGL.cpp
@@ +83,5 @@
>      int32_t stride = gfx::GetAlignedStride<4>(size.width * BytesPerPixel(format));
>      mData = gfx::Factory::CreateDataSourceSurfaceWithStride(size, format, stride);
> +    // Leave the extra return for clarity, in case we decide more code should
> +    // be added after this check, that should run even if mData is null.
> +    if (NS_WARN_IF(!mData)) {

I would rather this assert, since we don't claim to handle this properly, even if it happens to work right now.
I'm going to handle this in bug 1060044.
Blocks: 1060044
(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> ...
> > +    if (NS_WARN_IF(!mData)) {
> 
> I would rather this assert, since we don't claim to handle this properly,
> even if it happens to work right now.
> I'm going to handle this in bug 1060044.

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