Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7633038:
Relax software rendering list parsing. (Closed)
|
Created:
9 years, 4 months ago by Zhenyao Mo Modified:
9 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, jam, apatrick_chromium, Paweł Hajdan Jr. Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionRelax software rendering list parsing.
At the moment if we fail to process one entry, like encountering an unrecognized field, we fail the whole blacklist loading. This CL relaxes this behavior, so if we encounter unknown features, ignore them but keep the entry; if we encounter unknown fields in an entry, ignore the entry but keep the rest of the blacklist.
BUG=91872
TEST=unittest
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98162
Patch Set 1 #
Total comments: 2
Patch Set 2 : '' #
Total comments: 12
Patch Set 3 : '' #
Total comments: 10
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 2
Messages
Total messages: 14 (0 generated)
Please review.
Ping. Vangelis, this must have fallen out of your radar.
Sorry for the late review.. http://codereview.chromium.org/7633038/diff/1/content/browser/gpu/gpu_blackli... File content/browser/gpu/gpu_blacklist.cc (right): http://codereview.chromium.org/7633038/diff/1/content/browser/gpu/gpu_blackli... content/browser/gpu/gpu_blacklist.cc:649: continue; It seems to me that entries with unknown fields will be completely skipped, which is not what we need. We want them processed but their additional fields ignored. I believe GetGpuBlacklistEntryFromValue() needs to be modified to account for that and a test added.
http://codereview.chromium.org/7633038/diff/1/content/browser/gpu/gpu_blackli... File content/browser/gpu/gpu_blacklist.cc (right): http://codereview.chromium.org/7633038/diff/1/content/browser/gpu/gpu_blackli... content/browser/gpu/gpu_blacklist.cc:599: bool current_os_only, One more thing. Having two bool's next to each other in the function signature is confusing for the caller. It will be cleaner if they are converted to enums so the caller can do: LoadGpuBlacklist(context, kCurrentOSOnly, kIgnoreUnknownFields) or LoadGpuBlacklist(context, kAllOS, kFailOnUknownFields) or similar. And another nit: tolerate_errors is somewhat misleading. Maybe something like ignore_unknown_fields or similar?
On 2011/08/18 21:15:11, vangelis wrote: > http://codereview.chromium.org/7633038/diff/1/content/browser/gpu/gpu_blackli... > File content/browser/gpu/gpu_blacklist.cc (right): > > http://codereview.chromium.org/7633038/diff/1/content/browser/gpu/gpu_blackli... > content/browser/gpu/gpu_blacklist.cc:599: bool current_os_only, > One more thing. Having two bool's next to each other in the function signature > is confusing for the caller. It will be cleaner if they are converted to enums > so the caller can do: > > LoadGpuBlacklist(context, kCurrentOSOnly, kIgnoreUnknownFields) > > or > > LoadGpuBlacklist(context, kAllOS, kFailOnUknownFields) > > or similar. > > And another nit: tolerate_errors is somewhat misleading. Maybe something like > ignore_unknown_fields or similar? Revised according to review comments. A kIgnoreExceptionWithUnknownField flag is implemented and used. The reason is that we are very likely to relax our existing entries by adding a channel exception, so kIgnoreExceptionWithUnknownField will solve the backward compatibility issue. Please review again.
http://codereview.chromium.org/7633038/diff/15006/content/browser/gpu/gpu_bla... File content/browser/gpu/gpu_blacklist.cc (right): http://codereview.chromium.org/7633038/diff/15006/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:641: error = true; I find the use of the error variable is a bit confusing and fragile. What if you just set error = true only in situations where there is an error and you break out of the loop? That way you don't have to set its value twice in every iteration. http://codereview.chromium.org/7633038/diff/15006/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:665: if (unknown_field_option == kFailOnUnknownField) I think kFailOnUnknownField adds a lot of unnecessary complication to the code. Is it useful to ever fail parsing the blacklist because we encountered an unknown field or an unknown exception? I think that if we ever encounter an unknown field we should always discard the entry. If we encounter an unknown exception, ignore the exception and pretend it's not there. http://codereview.chromium.org/7633038/diff/15006/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:669: } else if (entry->contains_exception_with_unknown_field()) { Don't need an else if here. Previous if clause either breaks out of the loop of skips to the top again. http://codereview.chromium.org/7633038/diff/15006/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:671: delete entry; Don't need to delete the entry here. It will be taken care of outside the loop as error == true http://codereview.chromium.org/7633038/diff/15006/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:687: delete entries[i]; The management of entries is getting kind of hairy and error-prone. Can't entries be ref-counted so that you don't have to explicitly delete them? Entries that are actually used will be kept alive from a reference in blacklist_ and all other will automatically be deleted when the entries vector falls out of scope. http://codereview.chromium.org/7633038/diff/15006/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:704: for (size_t i = 0; i < entries.size(); ++i) You could avoid adding another deletion loop here by getting the os version at the very top of the function and bailing out early if it is kOsUnknown.
Revised. Please review again. http://codereview.chromium.org/7633038/diff/15006/content/browser/gpu/gpu_bla... File content/browser/gpu/gpu_blacklist.cc (right): http://codereview.chromium.org/7633038/diff/15006/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:641: error = true; On 2011/08/19 22:50:07, vangelis wrote: > I find the use of the error variable is a bit confusing and fragile. What if > you just set error = true only in situations where there is an error and you > break out of the loop? That way you don't have to set its value twice in every > iteration. Done. http://codereview.chromium.org/7633038/diff/15006/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:665: if (unknown_field_option == kFailOnUnknownField) On 2011/08/19 22:50:07, vangelis wrote: > I think kFailOnUnknownField adds a lot of unnecessary complication to the code. > Is it useful to ever fail parsing the blacklist because we encountered an > unknown field or an unknown exception? I think that if we ever encounter an > unknown field we should always discard the entry. If we encounter an unknown > exception, ignore the exception and pretend it's not there. Done. http://codereview.chromium.org/7633038/diff/15006/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:669: } else if (entry->contains_exception_with_unknown_field()) { On 2011/08/19 22:50:07, vangelis wrote: > Don't need an else if here. Previous if clause either breaks out of the loop of > skips to the top again. Done. http://codereview.chromium.org/7633038/diff/15006/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:671: delete entry; On 2011/08/19 22:50:07, vangelis wrote: > Don't need to delete the entry here. It will be taken care of outside the loop > as error == true Done. http://codereview.chromium.org/7633038/diff/15006/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:687: delete entries[i]; On 2011/08/19 22:50:07, vangelis wrote: > The management of entries is getting kind of hairy and error-prone. Can't > entries be ref-counted so that you don't have to explicitly delete them? > Entries that are actually used will be kept alive from a reference in blacklist_ > and all other will automatically be deleted when the entries vector falls out of > scope. Done. http://codereview.chromium.org/7633038/diff/15006/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:704: for (size_t i = 0; i < entries.size(); ++i) On 2011/08/19 22:50:07, vangelis wrote: > You could avoid adding another deletion loop here by getting the os version at > the very top of the function and bailing out early if it is kOsUnknown. Turns out we don't need to deal with kOsUnknown at all because kCurrentOsOnly already handles the filtering. Deleted the loop here.
I like the ref-counted pointer cleanup! Just a few more comments. http://codereview.chromium.org/7633038/diff/22005/content/browser/gpu/gpu_bla... File content/browser/gpu/gpu_blacklist.cc (right): http://codereview.chromium.org/7633038/diff/22005/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:630: bool error = false; Now that the code is so much simpler, you could eliminate the error variable and simply return instead of doing "error=true;break" http://codereview.chromium.org/7633038/diff/22005/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:659: contains_unknown_fields = true; I forget what we decided but I thought that unknown fields on entries would just be ignored. Wasn't it only the entries with unknown exceptions that would be skipped? For example, the presence of an unknown feature in the feature list should not cause the whole entry to be skipped. In any case, a small comment in the code that explains what the behavior is in the presence of unknown fields would be really helpful. http://codereview.chromium.org/7633038/diff/22005/content/browser/gpu/gpu_bla... File content/browser/gpu/gpu_blacklist.h (right): http://codereview.chromium.org/7633038/diff/22005/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.h:38: // In loading, Ignore all entries that belong to other OS. typo: Ignore->ignore http://codereview.chromium.org/7633038/diff/22005/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.h:235: // Returns true if an unknown filed of an unknown blacklist feature typo: filed -> field http://codereview.chromium.org/7633038/diff/22005/content/browser/gpu/gpu_bla... File content/browser/gpu/gpu_blacklist_unittest.cc (right): http://codereview.chromium.org/7633038/diff/22005/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist_unittest.cc:513: EXPECT_TRUE(blacklist.contains_unknown_fields()); It would be useful in these tests to blacklist different features (e.g. WebGL and canvas) in different entries to make sure that the entry skipping logic is correct.
Revised, please have another look. http://codereview.chromium.org/7633038/diff/22005/content/browser/gpu/gpu_bla... File content/browser/gpu/gpu_blacklist.cc (right): http://codereview.chromium.org/7633038/diff/22005/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:630: bool error = false; On 2011/08/24 19:00:17, vangelis wrote: > Now that the code is so much simpler, you could eliminate the error variable and > simply return instead of doing "error=true;break" Done. http://codereview.chromium.org/7633038/diff/22005/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:659: contains_unknown_fields = true; On 2011/08/24 19:00:17, vangelis wrote: > I forget what we decided but I thought that unknown fields on entries would just > be ignored. Wasn't it only the entries with unknown exceptions that would be > skipped? I think we decided the following: 1) ignore unknown features in a feature list 2) ignore unknown field inside an exception (so we could add channels as exceptions) 3) skip the entry if an unknown field exists outside an exception. For example, if we blacklist NVIDIA without an certain extension, and extension is a new added field, then we don't wanna simply ignore the extension field, because this will blacklist all NVIDIA. And yes, the current code's behavior also skip with an unknown field. So I am adding a new flag of contains_unknown_features to differentiate from contains_unknown_fields. > > For example, the presence of an unknown feature in the feature list should not > cause the whole entry to be skipped. > > > In any case, a small comment in the code that explains what the behavior is in > the presence of unknown fields would be really helpful. comments added. http://codereview.chromium.org/7633038/diff/22005/content/browser/gpu/gpu_bla... File content/browser/gpu/gpu_blacklist.h (right): http://codereview.chromium.org/7633038/diff/22005/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.h:38: // In loading, Ignore all entries that belong to other OS. On 2011/08/24 19:00:17, vangelis wrote: > typo: Ignore->ignore Done. http://codereview.chromium.org/7633038/diff/22005/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.h:235: // Returns true if an unknown filed of an unknown blacklist feature On 2011/08/24 19:00:17, vangelis wrote: > typo: filed -> field Done. http://codereview.chromium.org/7633038/diff/22005/content/browser/gpu/gpu_bla... File content/browser/gpu/gpu_blacklist_unittest.cc (right): http://codereview.chromium.org/7633038/diff/22005/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist_unittest.cc:513: EXPECT_TRUE(blacklist.contains_unknown_fields()); On 2011/08/24 19:00:17, vangelis wrote: > It would be useful in these tests to blacklist different features (e.g. WebGL > and canvas) in different entries to make sure that the entry skipping logic is > correct. Done.
Revised again, so now it always skips a entry if it contains an unknown field, inside or outside an exception; it always ignores an unknown feature. Please review.
LGTM Thanks for all the iterations! http://codereview.chromium.org/7633038/diff/34002/content/browser/gpu/gpu_bla... File content/browser/gpu/gpu_blacklist.h (right): http://codereview.chromium.org/7633038/diff/34002/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.h:319: // Check if any entries contain unknown field. This is only for tests. typo: field -> fields
http://codereview.chromium.org/7633038/diff/34002/content/browser/gpu/gpu_bla... File content/browser/gpu/gpu_blacklist.cc (right): http://codereview.chromium.org/7633038/diff/34002/content/browser/gpu/gpu_bla... content/browser/gpu/gpu_blacklist.cc:423: return entry.release(); This is the leak. The ScopedGpuBlacklistEntry constructor above gives the GpuBlacklistEntry a ref count of 1. This release call does *not* adjust the reference count, just sets the referent to NULL, so the returned pointer still has a reference count of 1. The caller wraps the return value in a ScopedGpuBlacklistEntry, incrementing its reference count again (to 2) and when that wrapper is later deleted the refcount only goes down to 1, never 0, so all of the blacklist entries are being leaked. The fix is to change the return value of this function to a ScopedGpuBlacklistEntry.
On 2011/08/25 18:03:31, kbr wrote: > http://codereview.chromium.org/7633038/diff/34002/content/browser/gpu/gpu_bla... > File content/browser/gpu/gpu_blacklist.cc (right): > > http://codereview.chromium.org/7633038/diff/34002/content/browser/gpu/gpu_bla... > content/browser/gpu/gpu_blacklist.cc:423: return entry.release(); > This is the leak. The ScopedGpuBlacklistEntry constructor above gives the > GpuBlacklistEntry a ref count of 1. This release call does *not* adjust the > reference count, just sets the referent to NULL, so the returned pointer still > has a reference count of 1. The caller wraps the return value in a > ScopedGpuBlacklistEntry, incrementing its reference count again (to 2) and when > that wrapper is later deleted the refcount only goes down to 1, never 0, so all > of the blacklist entries are being leaked. The fix is to change the return value > of this function to a ScopedGpuBlacklistEntry. Good catch! Thanks, Ken.
On 2011/08/25 18:03:31, kbr wrote: > http://codereview.chromium.org/7633038/diff/34002/content/browser/gpu/gpu_bla... > File content/browser/gpu/gpu_blacklist.cc (right): > > http://codereview.chromium.org/7633038/diff/34002/content/browser/gpu/gpu_bla... > content/browser/gpu/gpu_blacklist.cc:423: return entry.release(); > This is the leak. The ScopedGpuBlacklistEntry constructor above gives the > GpuBlacklistEntry a ref count of 1. This release call does *not* adjust the > reference count, just sets the referent to NULL, so the returned pointer still > has a reference count of 1. The caller wraps the return value in a > ScopedGpuBlacklistEntry, incrementing its reference count again (to 2) and when > that wrapper is later deleted the refcount only goes down to 1, never 0, so all > of the blacklist entries are being leaked. The fix is to change the return value > of this function to a ScopedGpuBlacklistEntry. Ah, I was confused this release with the release in the ref_counted class, and thought both of them would reduce the ref count. Thanks a lot Ken! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
