Closed Bug 1068613 Opened 5 years ago Closed 5 years ago

Add Moz2D allocation failures to crash reports, with error codes.

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: nical, Assigned: nical)

Details

Attachments

(4 files, 3 obsolete files)

We need some info about texture allocation failures in crash reports to better understand how much of the issue is OOM and if there are also cases where failure can be caused by invalid states somewhere. We specifically need this info in the very short term so let's separate this from bug 1065399.
This uses the same mechanism as gfxWarning() and friends, but deffers the work to an ErrorReporter that has to be implemented externally to not pull in gecko dependencies into Moz2D.
Assignee: nobody → nical.bugzilla
Attachment #8490738 - Flags: review?(bas)
I don't know if there is a specific format for app notes, so I may be completely off with this patch. The strings logged look like:

"[D2D1.1] CreateBitmap failure Size(255,255) Code: 1234567\n"

see patch part 3
Attachment #8490743 - Flags: review?(benjamin)
Attachment #8490743 - Flags: review?(bas)
Attachment #8490744 - Flags: review?(bas)
Comment on attachment 8490743 [details] [diff] [review]
part 2) Add an ErrorReporter that appends the error to app notes in the crash report

I don't understand why gfx::ErrorReporter::ShutDown has to be in nsXPComInit instead of gfxPlatform::Shutdown or some other gfx-specific location.

The app notes is intended to be a human-readable free-form field. It must not contain data that might be privacy-sensitive.

If you are logging specific error information, please consider using a custom annotation instead of the app-notes field. That will make it easier to search on crash-stats and produce custom reports.

I don't think this patch by itself provides enough detail for me to understand the data which might be included to decide whether this is a good use of app-notes or not.
Attachment #8490743 - Flags: review?(benjamin)
Comment on attachment 8490738 [details] [diff] [review]
Add gfxCriticalError() lof to Moz2D

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

::: gfx/2d/Logging.h
@@ +75,4 @@
>    }
> +};
> +
> +struct ErrorReporter {

I'd like this to get a more generic name, so we can use it for other logging stuff.

@@ +88,5 @@
>    }
> +  static ErrorReporter* Get() { return sReporter; }
> +
> +private:
> +  static ErrorReporter* sReporter;

Don't make this a static, have it set on the factory.
Comment on attachment 8490743 [details] [diff] [review]
part 2) Add an ErrorReporter that appends the error to app notes in the crash report

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

This looks fine taking the corrections in the first class into account.
Attachment #8490743 - Flags: review?(bas) → review+
Comment on attachment 8490744 [details] [diff] [review]
part 3) Log allocation failures in D2D and D2D1.1

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

You need to make sure we log a texture creation failure inside TextureD3D11 as well.
Attachment #8490738 - Attachment is obsolete: true
Attachment #8490738 - Flags: review?(bas)
Attachment #8490863 - Flags: review?(bas)
Moved the ShutDown into gfxPlatform::Shutdown. I originally put it in ShutdownXPCOM to be 100% sure that no moz2d resource was still alive (TextureClients can be kept alive by media code longer than gfxPlatform), but for the purpose of what we are logging here it doesn't matter.

This patch is still using App Notes. I looked a bit but it doesn't look trivial to add another section (trivial as in we would like to land this asap to get a chance to collect some info and fix bugs by Friday). I propose that we keep this in app notes temporarily and add a critical errors section in the crash stats afterwards.

What this log will typically output is a few lines as the one I showed earlier, only if the user ran into a "critical error" path before crashing (and right now those critical errors are failure from the low level API (D2D/D3D) to allocate a Texture on the GPU, which we haven't managed to reproduce on our ends).
Attachment #8490743 - Attachment is obsolete: true
Attachment #8490871 - Flags: review?(bas)
Attachment #8490871 - Flags: feedback?(benjamin)
Attachment #8490744 - Attachment is obsolete: true
Attachment #8490744 - Flags: review?(bas)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Comment on attachment 8490743 [details] [diff] [review]
> part 2) Add an ErrorReporter that appends the error to app notes in the
> crash report
> 
> I don't understand why gfx::ErrorReporter::ShutDown has to be in nsXPComInit
> instead of gfxPlatform::Shutdown or some other gfx-specific location.
> 
> The app notes is intended to be a human-readable free-form field. It must
> not contain data that might be privacy-sensitive.
> 
> If you are logging specific error information, please consider using a
> custom annotation instead of the app-notes field. That will make it easier
> to search on crash-stats and produce custom reports.
> 
> I don't think this patch by itself provides enough detail for me to
> understand the data which might be included to decide whether this is a good
> use of app-notes or not.

Basically, within a day or 2, we need more data on whether some of the crashes we're seeing are OOMs or not. The data we intend to add here is when a surface creation fails, the error code windows returns and the size of the surfaces that was attempted to be created. I doubt this will be privacy sensitive.

Fwiw I think a separate area for annotating crash reports with the gfx Warning/Error log would be nicer, but I feel since we need this data fairly quickly this is the best short term solution? Unless there's something else useful we could do short-term?
Flags: needinfo?(benjamin)
Attachment #8490863 - Attachment is patch: true
ok
Flags: needinfo?(benjamin)
Attachment #8490863 - Flags: review?(bas) → review+
Attachment #8490871 - Flags: review?(bas) → review+
Attachment #8490872 - Flags: review?(bas) → review+
This will help us see if our surfaces are in an error state when CreateSimilar is called. This should be helpful for bug 1015683.
Attachment #8491038 - Flags: review?(jmuizelaar)
Attachment #8491038 - Flags: review?(jmuizelaar) → review+
Attachment #8490871 - Flags: feedback?(benjamin)
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.