1152331 - gfxCriticalError/LogFailure interferes with parts of about:support graphics

Status: RESOLVED FIXED

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

If LogFailure or any other gfxCriticalError fires, we get the message in graphics section of about support; however, we lose parts of the standard information in that section, like the graphics card, WebGL info, etc.  This was bad from the time this landed with bug 1112828.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Save the whole variable, not just the .value.

Somebody that understands JS/C++ interaction better can explain the underlying mechanism, but it seems unsafe to keep a .value of a returned IPDL array, as it may lead to a memory corruption.
David, if you understand this, great, you can review it and tell me if it makes sense; if you don't, any suggestions as to who a good person for it would be?

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Try as a sanity check. https://treeherder.mozilla.org/#/jobs?repo=try&revision=86341f71427e

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

I want to take a look at this a bit more.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Leaving data.indices around breaks the display of the rest of data. r=dvander

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Leaving data.indices around breaks the display of the rest of data. r=dvander

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Planning on asking for the uplift of this to 38 & 39, so review with extra care :)

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8590286 [details] [diff] [review]
Leaving data.indices around breaks the display of the rest of data. r=dvander

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

Looks right to me. This file is pretty crazy. It would have been a lot simpler to copy the properties we want than create a prototype chain and delete the ones we don't... but alas.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to David Anderson [:dvander] from comment #7)
> Looks right to me. This file is pretty crazy. It would have been a lot
> simpler to copy the properties we want than create a prototype chain and
> delete the ones we don't... but alas.

Agreed.  Filed one follow up bug to start, but wanted this patch simple enough that we could uplift it if approved.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=185dce5a2a97

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8590286 [details] [diff] [review]
Leaving data.indices around breaks the display of the rest of data. r=dvander

Approval Request Comment
[Feature/regressing bug #]: 1112828 (in 38)
[User impact if declined]: If a graphics error is encountered about:support will contain it, but not contain the standard information about the user's system. This will give us less useful information from the users that do encounter this problem, so all the direct impact is on us, not getting the correct data.  Note this does not affect the crash reports or telemetry, just the info displayed in about:support.
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: Low to none.  We are removing the field that we were not supposed to be processing down the line, nor do we process in the scenario where there are no errors to be reported.
[String/UUID change made/needed]: n/a

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45d577adc4b0

Comment 12

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/45d577adc4b0

Comment 13

[Sylvestre Ledru [:sylvestre]]

Comment on attachment 8590286 [details] [diff] [review]
Leaving data.indices around breaks the display of the rest of data. r=dvander

Should be in 38 beta 5

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f29b348372d3

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4cc36a9a958b