Closed Bug 1263249 Opened 4 years ago Closed 4 years ago

Bubble up unique failureId in GetFeatureStatus

Categories

(Core :: Graphics, defect)

Unspecified
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: BenWa, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

I'd like to introduce a unique failureId. This failureId will let us know what triggered the feature to be blocked. Later I'd like to send it to the telemetry so that we can query against it.
Assignee: nobody → bgirard
OS: Unspecified → All
Whiteboard: [gfx-noted]
I've got all the mac and cross-platform failures annotated. Need to wrap up the other platform specific ones.

I noticed that we hit an early return on mac in the blocklist test. I wonder if it's like this across all platforms:
|if (!(gfxInfo instanceof Ci.nsIGfxInfoDebug)) {|

Doesn't look like it's expected either since there's some mac specific spoofing in the test files.
Right - the automated tests for the blocklists is currently debug only.
I just remembered that I only tested this on desktop. Will probably need another patch for Fennec.
Comment on attachment 8741143 [details]
MozReview Request: Bug 1263249 - Bubble up unique failureId in GetFeatureStatus. r=milan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46229/diff/1-2/
Details coming, but one thing that stands out a bit is a mixture of nsString, nsCString, char*, places where we pass char** and nullptr is OK, places where we pass char** and nullptr is not OK, places with char*&, nsString*, and it takes some involved chasing to make sure nothing is leaking, double freed or a nullptr dereference.  Maybe there is a way to standardize on one of these types (nsCString?) and make things more explicit and robust?  Make it simpler for the next person touching this code :)
Yes you're right. I'm not a fan of how this turned out. ns*String is used when we're interfacting with IPDL. char* comes from our IDL bindings for the string type. I can switch everything to nsCString as much as possible to make things a bit cleaner.
Comment on attachment 8741143 [details]
MozReview Request: Bug 1263249 - Bubble up unique failureId in GetFeatureStatus. r=milan

https://reviewboard.mozilla.org/r/46229/#review43001

I think we need to sort out the strings issues, who owns it, whether we should be using char**, etc.

::: dom/ipc/ContentParent.cpp:4899
(Diff revision 2)
>      return true;
>    }
>  
> -  *aSuccess = NS_SUCCEEDED(gfxInfo->GetFeatureStatus(aFeature, aStatus));
> +  char* failureId;
> +  *aSuccess = NS_SUCCEEDED(gfxInfo->GetFeatureStatus(aFeature, &failureId, aStatus));
> +  *aFailureId = NS_ConvertUTF8toUTF16(failureId);

aFailureId could be nullptr?  If not, maybe a & instead of *?

::: dom/media/android/AndroidMediaPluginHost.cpp:116
(Diff revision 2)
>    ScopedGfxFeatureReporter reporter("Stagefright", forceEnabled);
>  
>    if (!forceEnabled) {
>      nsCOMPtr<nsIGfxInfo> gfxInfo = services::GetGfxInfo();
>      if (gfxInfo) {
> +      char** discardFailure = nullptr;

I like the explicit "discardFailure" instead of just passing nullptr to GetFeatureStatus.  Makes it more explicit.

::: gfx/thebes/gfxUtils.cpp:1550
(Diff revision 2)
>        dom::workers::GetCurrentThreadWorkerPrivate();
>      RefPtr<GetFeatureStatusRunnable> runnable =
> -      new GetFeatureStatusRunnable(workerPrivate, gfxInfo, feature, status);
> +      new GetFeatureStatusRunnable(workerPrivate, gfxInfo, feature, status,
> +                                   failureId);
> +
> +    *failureId = nullptr;

It seems failureId is assumed to not be nullptr in the ThreadSafeGetFeatureStatus, yet GetFeatureStatus itself allows nullptr.  Is there a reason for the inconsistency?

::: toolkit/mozapps/extensions/test/xpcshell/data/test_gfxBlacklist.xml:5
(Diff revision 2)
>  <?xml version="1.0" encoding="UTF-8"?>
>  
>  <blocklist xmlns="http://www.mozilla.org/2006/addons-blocklist">
>    <gfxItems>
> -    <gfxBlacklistEntry>
> +    <gfxBlacklistEntry blockID="g35">

Do we have tests when the blocklist entry does not, for some reason, include the blockID?  We should.

::: widget/GfxInfoBase.h:48
(Diff revision 2)
>    // Derived classes need to use
>    // using GfxInfoBase::GetFeatureStatus;
>    // using GfxInfoBase::GetFeatureSuggestedDriverVersion;
>    // using GfxInfoBase::GetWebGLParameter;
>    // to import the relevant methods into their namespace.
> -  NS_IMETHOD GetFeatureStatus(int32_t aFeature, int32_t *_retval) override;
> +  NS_IMETHOD GetFeatureStatus(int32_t aFeature, char** aFailureId, int32_t *_retval) override;

We should be explicit about aFailureId, if it can be nullptr, who owns the string afterwards, in charge of deleting it, etc.

Are we better off not using char** and instead having strings instead?  Like the other methods?  Would that make ownership more explicit and less prone to errors/leaks?

::: widget/GfxInfoBase.cpp:994
(Diff revision 2)
>    if (NS_FAILED(GetAdapterVendorID(adapterVendorID)) ||
>        NS_FAILED(GetAdapterDeviceID(adapterDeviceID)) ||
>        NS_FAILED(GetAdapterDriverVersion(adapterDriverVersionString)))
>    {
> +    if (aFailureId) {
> +      *aFailureId = strdup("FEATURE_FAILURE_CANT_RESOLVE_ADAPTER");

We are assuming that the base class GetFeatureStatusImpl will not get this far if there was a failure id already set, because of the code a few lines above (returning if status is not unknown), but that's strong coupling the two.  I would still handle the case where *aFailureId is not null somehow (perhaps an assert is enough), because otherwise we'd have a leak.

::: widget/GfxInfoX11.cpp:395
(Diff revision 2)
>          // Bug 724640: FGLRX + Linux 2.6.32 is a crashy combo
>          bool unknownOS = mOS.IsEmpty() || mOSRelease.IsEmpty();
>          bool badOS = mOS.Find("Linux", true) != -1 &&
>                       mOSRelease.Find("2.6.32") != -1;
>          if (unknownOS || badOS) {
> +          if (aFailureId) {

Feels like
if (aFeatureId) {
  *aFeatureId = strdup(...);
}
could be a good candidate for an inline helper function, for readability...

::: widget/android/GfxInfo.cpp:376
(Diff revision 2)
>  {
>    if (mDriverInfo->IsEmpty()) {
>      APPEND_TO_DRIVER_BLOCKLIST2( DRIVER_OS_ALL,
>        (nsAString&) GfxDriverInfo::GetDeviceVendor(VendorAll), GfxDriverInfo::allDevices,
>        nsIGfxInfo::FEATURE_OPENGL_LAYERS, nsIGfxInfo::FEATURE_STATUS_OK,
> -      DRIVER_COMPARISON_IGNORED, GfxDriverInfo::allDriverVersions );
> +      DRIVER_COMPARISON_IGNORED, GfxDriverInfo::allDriverVersions, "FEATURE_OK_FORCE_OPENGL" );

In general, is there a reason these ids/descriptions are "no white space" and look like enums?  Rather than "Feature OK: Force OpenGL", for example?
Attachment #8741143 - Flags: review?(milan)
(In reply to Benoit Girard (:BenWa) from comment #7)
> Yes you're right. I'm not a fan of how this turned out. ns*String is used
> when we're interfacting with IPDL. char* comes from our IDL bindings for the
> string type. I can switch everything to nsCString as much as possible to
> make things a bit cleaner.

Sorry, some of my comments are because I didn't see this update :)
(In reply to Milan Sreckovic [:milan] from comment #8)
> In general, is there a reason these ids/descriptions are "no white space"
> and look like enums?  Rather than "Feature OK: Force OpenGL", for example?

My aim was to have id/identifier tokens. I'd rather not have spaces and not have them be sentences but we could use something like eFeatureFailureForceOpenGL if we want. This way they should be easy to find if they look like unique tokens. I just want something where we aggregate using a stable identifier. The blocklist code is complex so it's best if we have something where we can easily lookup the logic.

But if we wanted to present these to the user we would need to have a mapping.
Alright turns out you can use nsACString in IDL so I made all the code safe. There's going to be extra copies but that's fine.
Comment on attachment 8741143 [details]
MozReview Request: Bug 1263249 - Bubble up unique failureId in GetFeatureStatus. r=milan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46229/diff/2-3/
Attachment #8741143 - Flags: review?(milan)
Comment on attachment 8741143 [details]
MozReview Request: Bug 1263249 - Bubble up unique failureId in GetFeatureStatus. r=milan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46229/diff/3-4/
Comment on attachment 8741143 [details]
MozReview Request: Bug 1263249 - Bubble up unique failureId in GetFeatureStatus. r=milan

https://reviewboard.mozilla.org/r/46229/#review43719

::: dom/media/platforms/android/AndroidDecoderModule.cpp:269
(Diff revision 4)
>        aMimeType.EqualsLiteral("audio/wave; codecs=7") ||
>        aMimeType.EqualsLiteral("audio/wave; codecs=65534")) {
>      return false;
>    }  
>  
> +  nsCString discardFailureId;

Unused?

::: gfx/thebes/gfxUtils.cpp:1504
(Diff revision 4)
>  public:
>      GetFeatureStatusRunnable(dom::workers::WorkerPrivate* workerPrivate,
>                               const nsCOMPtr<nsIGfxInfo>& gfxInfo,
>                               int32_t feature,
> -                             int32_t* status)
> +                             int32_t* status,
> +                             nsACString& failureId)

Nit: majority (all other?) places have failureId parameter between feature and status, with the status remaining as the last argument.  This is one place where that's not the case.

::: widget/GfxDriverInfo.cpp:32
(Diff revision 4)
>      mFeatureStatus(nsIGfxInfo::FEATURE_STATUS_OK),
>      mComparisonOp(DRIVER_COMPARISON_IGNORED),
>      mDriverVersion(0),
>      mDriverVersionMax(0),
>      mSuggestedVersion(nullptr),
> +    mRuleId(nullptr),

This is safe on CStrings, right?

::: widget/GfxInfoBase.cpp:647
(Diff revision 4)
> +    nsCString blockIdStr = NS_LITERAL_CSTRING("FEATURE_FAILURE_DL_BLACKLIST_") +
> +                           NS_ConvertUTF16toUTF8(dataValue);
> +    aDriverInfo.mRuleId = blockIdStr.get();
> +  } else {
> +    nsCString blockIdStr = NS_LITERAL_CSTRING("FEATURE_FAILURE_DL_BLACKLIST_NO_ID");
> +    aDriverInfo.mRuleId = blockIdStr.get();

Can we just do:
aDriverInfo.mRuleId = NS_LITERAL_CSTRING(...);

::: widget/android/GfxInfo.cpp:418
(Diff revision 4)
>    // Don't evaluate special cases when evaluating the downloaded blocklist.
>    if (aDriverInfo.IsEmpty()) {
>      if (aFeature == nsIGfxInfo::FEATURE_CANVAS2D_ACCELERATION) {
>        // It's slower than software due to not having a compositing fast path
>        *aStatus = (mSDKVersion >= 11) ? nsIGfxInfo::FEATURE_STATUS_OK : nsIGfxInfo::FEATURE_BLOCKED_OS_VERSION;
> +      if (*aStatus == nsIGfxInfo::FEATURE_BLOCKED_OS_VERSION) {

You use a slightly different pattern below, it may fit better here as well:
if (mSDKVersion >= 11) {
  *aStatus = nsIGfxInfo::FEATURE_STATUS_OK;
} else {
  *aStatus = nsIGFxInfo::FEATURE_BLOCKED_OS_VERSION;
  aFailuresId = "FEATURE_FAILURE_CANVAS_2D_SDK";
}

::: widget/android/GfxInfo.cpp:429
(Diff revision 4)
>      if (aFeature == FEATURE_WEBGL_OPENGL) {
>        if (mGLStrings->Renderer().Find("Adreno 200") != -1 ||
>            mGLStrings->Renderer().Find("Adreno 205") != -1)
>        {
>          *aStatus = nsIGfxInfo::FEATURE_BLOCKED_DEVICE;
> +        if (*aStatus == nsIGfxInfo::FEATURE_BLOCKED_OS_VERSION) {

Don't think this is what you meant to do, but I can't tell what you meant to do.  Are we to set aFailureId to "FEATURE_FAILURE_ADRENO_20x" all the time?  The if will never trigger, we just set *aStatus to something else.  Or are we supposed to keep the if, but do the test before *aStatus gets reset?

Same thing in the next code block and "ville" test.

::: widget/android/GfxInfo.cpp:621
(Diff revision 4)
>          // GIFV crash, see bug 1232911.
>          model.Equals("GT-N8013", nsCaseInsensitiveCStringComparator());
>  
>        *aStatus = isBlocked ? nsIGfxInfo::FEATURE_BLOCKED_DEVICE
>                             : nsIGfxInfo::FEATURE_STATUS_OK;
> +      if (isBlocked) {

Another place where the pattern of assigning the value of *aStatus inside the regular if/else may fit better.

::: widget/cocoa/GfxInfo.mm:354
(Diff revision 4)
>          *aStatus = nsIGfxInfo::FEATURE_STATUS_OK;
>          return NS_OK;
>        }
>      } else if (aFeature == nsIGfxInfo::FEATURE_CANVAS2D_ACCELERATION) {
>        // See bug 1249659
> -      *aStatus = (os > DRIVER_OS_OS_X_10_7) ? nsIGfxInfo::FEATURE_STATUS_OK : nsIGfxInfo::FEATURE_BLOCKED_OS_VERSION;
> +      if (os > DRIVER_OS_OS_X_10_7) {

This is the pattern I was referencing above as perhaps more appropriate.
Attachment #8741143 - Flags: review?(milan) → review+
(In reply to Milan Sreckovic [:milan] from comment #14)
> > +    mRuleId(nullptr),
> 
> This is safe on CStrings, right?

Yes, nsTSubstring_CharT::Assign will check !aData

> 
> ::: widget/android/GfxInfo.cpp:429
> (Diff revision 4)
> >      if (aFeature == FEATURE_WEBGL_OPENGL) {
> >        if (mGLStrings->Renderer().Find("Adreno 200") != -1 ||
> >            mGLStrings->Renderer().Find("Adreno 205") != -1)
> >        {
> >          *aStatus = nsIGfxInfo::FEATURE_BLOCKED_DEVICE;
> > +        if (*aStatus == nsIGfxInfo::FEATURE_BLOCKED_OS_VERSION) {
> 
> Don't think this is what you meant to do, but I can't tell what you meant to
> do.  Are we to set aFailureId to "FEATURE_FAILURE_ADRENO_20x" all the time? 
> The if will never trigger, we just set *aStatus to something else.  Or are
> we supposed to keep the if, but do the test before *aStatus gets reset?
> 
> Same thing in the next code block and "ville" test.

Opps, just a copy paste error. The IF shouldn't be there. Thanks for the thorough review.
Comment on attachment 8741143 [details]
MozReview Request: Bug 1263249 - Bubble up unique failureId in GetFeatureStatus. r=milan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46229/diff/4-5/
Comment on attachment 8741143 [details]
MozReview Request: Bug 1263249 - Bubble up unique failureId in GetFeatureStatus. r=milan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46229/diff/5-6/
Comment on attachment 8741143 [details]
MozReview Request: Bug 1263249 - Bubble up unique failureId in GetFeatureStatus. r=milan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46229/diff/6-7/
Somebody else should review the ContentParent/IPDL changes, I'm not sure what all those tokens are.
Comment on attachment 8741143 [details]
MozReview Request: Bug 1263249 - Bubble up unique failureId in GetFeatureStatus. r=milan

mconley might be a good fit to review this. Can you look at the ContentParent/IPDL changes?
Flags: needinfo?(mconley)
Attachment #8741143 - Flags: review?(mconley)
Comment on attachment 8741143 [details]
MozReview Request: Bug 1263249 - Bubble up unique failureId in GetFeatureStatus. r=milan

https://reviewboard.mozilla.org/r/46229/#review43965

IPDL changes look fine.
Attachment #8741143 - Flags: review?(mconley) → review+
Depends on: 1266320
https://hg.mozilla.org/mozilla-central/rev/e0d5bf97a182
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.