Closed
Bug 1263249
Opened 8 years ago
Closed 8 years ago
Bubble up unique failureId in GetFeatureStatus
Categories
(Core :: Graphics, defect)
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.
Updated•8 years ago
|
Assignee: nobody → bgirard
OS: Unspecified → All
Whiteboard: [gfx-noted]
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46229/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46229/
Attachment #8741143 -
Flags: review?(milan)
Assignee | ||
Comment 4•8 years ago
|
||
I just remembered that I only tested this on desktop. Will probably need another patch for Fennec.
Assignee | ||
Comment 5•8 years ago
|
||
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 :)
Assignee | ||
Comment 7•8 years ago
|
||
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 :)
Assignee | ||
Comment 10•8 years ago
|
||
(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.
Assignee | ||
Comment 11•8 years ago
|
||
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.
Assignee | ||
Comment 12•8 years ago
|
||
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)
Assignee | ||
Comment 13•8 years ago
|
||
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+
Assignee | ||
Comment 15•8 years ago
|
||
(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.
Assignee | ||
Comment 16•8 years ago
|
||
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/
Assignee | ||
Comment 17•8 years ago
|
||
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/
Assignee | ||
Comment 18•8 years ago
|
||
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.
Assignee | ||
Comment 21•8 years ago
|
||
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 22•8 years ago
|
||
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+
Updated•8 years ago
|
Flags: needinfo?(mconley)
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e0d5bf97a182
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•