Bug 202573

Created attachment 380629 [details] Patch

Does this patch depend on something else that isn't landed yet? EWS couldn't process it.

(In reply to Alexey Proskuryakov from comment #2) > Does this patch depend on something else that isn't landed yet? EWS couldn't > process it. Yes, thanks for the reminder to update the depends field.

Created attachment 384892 [details] Patch

Comment on attachment 384892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384892&action=review I don’t see tests for all the different errors that convertToBlob can produce. > Source/WebCore/html/OffscreenCanvas.cpp:183 > +static Optional<double> qualityFromDouble(double qualityNumber) > +{ > + if (qualityNumber < 0 || qualityNumber > 1) > + return WTF::nullopt; > + > + return qualityNumber; > +} This will return a NaN if passed a NaN. I suggest writing this instead: if (!(qualityNumber >= 0 && qualityNumber <= 1)) return WTF::nullopt; That should return nullopt if passed a NaN. And because it’s "unrestricted double" I think the value *can* be a NaN. But that doesn’t seem like it’s covered by tests. Are there tests that cover values outside the 0.0-1.0 range? > Source/WebCore/html/OffscreenCanvas.cpp:188 > + promise->reject(SecurityError); I don’t see a test for this case. > Source/WebCore/html/OffscreenCanvas.cpp:196 > + promise->reject(InvalidStateError); I don’t see a test for this case. > Source/WebCore/html/OffscreenCanvas.cpp:207 > + promise->reject(EncodingError); I don’t see a test for this case. > Source/WebCore/html/OffscreenCanvas.cpp:212 > + promise->resolveWithNewlyCreated<IDLInterface<Blob>>(blob); I think we can remove one reference count churn by writing WTFMove(blob) here. > Source/WebCore/platform/MIMETypeRegistry.cpp:494 > + return createSupportedImageMIMETypesForEncoding().contains(mimeType); This implementation likely doesn’t have acceptable performance. The Cocoa version of createSupportedImageMIMETypesForEncoding is quite inefficient and I suspect it’s not OK to call it repeatedly every time we process an image MIME type. There may be another solution to making it work on multiple threads that avoids this inefficiency. There’s no need to alter String reference counts just to check if a String is in a map, so we might be able to make the idiom work where the set is created on the main thread and it’s safe to read it from any thread since it’s never modified.

(In reply to Darin Adler from comment #5) > Comment on attachment 384892 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=384892&action=review > > I don’t see tests for all the different errors that convertToBlob can > produce. > > > Source/WebCore/html/OffscreenCanvas.cpp:183 > > +static Optional<double> qualityFromDouble(double qualityNumber) > > +{ > > + if (qualityNumber < 0 || qualityNumber > 1) > > + return WTF::nullopt; > > + > > + return qualityNumber; > > +} > > This will return a NaN if passed a NaN. I suggest writing this instead: > > if (!(qualityNumber >= 0 && qualityNumber <= 1)) > return WTF::nullopt; > > That should return nullopt if passed a NaN. Nice catch. I pretty much copied this from qualityFromJSValue in HTMLCanvasElement.cpp, would that have the same issue? > And because it’s "unrestricted double" I think the value *can* be a NaN. But > that doesn’t seem like it’s covered by tests. Are there tests that cover > values outside the 0.0-1.0 range? > > > Source/WebCore/html/OffscreenCanvas.cpp:188 > > + promise->reject(SecurityError); > > I don’t see a test for this case. I think this is missing, I'll need to follow up in wpt. > > Source/WebCore/html/OffscreenCanvas.cpp:196 > > + promise->reject(InvalidStateError); > > I don’t see a test for this case. I believe this is tested in imported/w3c/web-platform-tests/offscreen-canvas/convert-to-blob/offscreencanvas.convert.to.blob(.w).html > > Source/WebCore/html/OffscreenCanvas.cpp:207 > > + promise->reject(EncodingError); > > I don’t see a test for this case. Same here, will follow up in wpt. > > Source/WebCore/html/OffscreenCanvas.cpp:212 > > + promise->resolveWithNewlyCreated<IDLInterface<Blob>>(blob); > > I think we can remove one reference count churn by writing WTFMove(blob) > here. I'm not sure why I didn't just write this as one line... > > Source/WebCore/platform/MIMETypeRegistry.cpp:494 > > + return createSupportedImageMIMETypesForEncoding().contains(mimeType); > > This implementation likely doesn’t have acceptable performance. The Cocoa > version of createSupportedImageMIMETypesForEncoding is quite inefficient and > I suspect it’s not OK to call it repeatedly every time we process an image > MIME type. > > There may be another solution to making it work on multiple threads that > avoids this inefficiency. There’s no need to alter String reference counts > just to check if a String is in a map, so we might be able to make the idiom > work where the set is created on the main thread and it’s safe to read it > from any thread since it’s never modified. Yes, I suppose the main-thread assertion could just be removed for isSupportedImageMIMETypesForEncoding? It doesn't alter anything that I can see...

Comment on attachment 384892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384892&action=review >>> Source/WebCore/html/OffscreenCanvas.cpp:183 >>> +} >> >> This will return a NaN if passed a NaN. I suggest writing this instead: >> >> if (!(qualityNumber >= 0 && qualityNumber <= 1)) >> return WTF::nullopt; >> >> That should return nullopt if passed a NaN. >> >> And because it’s "unrestricted double" I think the value *can* be a NaN. But that doesn’t seem like it’s covered by tests. Are there tests that cover values outside the 0.0-1.0 range? > > Nice catch. I pretty much copied this from qualityFromJSValue in HTMLCanvasElement.cpp, would that have the same issue? Yes, from code inspection alone, I think it would apply in HTMLCanvasElement’s toDataURL DOM function, too. Really the same issue: need a test case to be sure of what the behavior is with NaN and I am not sure we have one. Using an "any" argument means a lot of things might be wrong. Wonder if the intended behavior happens if you pass a boolean false, for example. Here in ImageEncodeOptions a false would turn into a 0, but probably not there. >>> Source/WebCore/html/OffscreenCanvas.cpp:196 >>> + promise->reject(InvalidStateError); >> >> I don’t see a test for this case. > > I believe this is tested in imported/w3c/web-platform-tests/offscreen-canvas/convert-to-blob/offscreencanvas.convert.to.blob(.w).html Is it the one that says "Test that call convertToBlob on a detached OffscreenCanvas throws exception"? If so, then it’s still failing. So I suppose this will be covered once we fix the rest of what is making that fail. >>> Source/WebCore/platform/MIMETypeRegistry.cpp:494 >>> + return createSupportedImageMIMETypesForEncoding().contains(mimeType); >> >> This implementation likely doesn’t have acceptable performance. The Cocoa version of createSupportedImageMIMETypesForEncoding is quite inefficient and I suspect it’s not OK to call it repeatedly every time we process an image MIME type. >> >> There may be another solution to making it work on multiple threads that avoids this inefficiency. There’s no need to alter String reference counts just to check if a String is in a map, so we might be able to make the idiom work where the set is created on the main thread and it’s safe to read it from any thread since it’s never modified. > > Yes, I suppose the main-thread assertion could just be removed for isSupportedImageMIMETypesForEncoding? It doesn't alter anything that I can see... Maybe. We couldn’t use it exactly as is, because we’d need to use a thread-safe way of initializing the global. C++ provides thread safety for initializing globals in general, but the WebKit project uses a compiler option that turns off that thread safety. Need to use call_once explicitly instead. Then we’d need to audit potential thread safety issues in the HashMap::contains function. If the code does any reference count incrementing/decrementing at all, even by accident, then it would be a thread safety problem. Inspection of the reference count is safe, though. I wonder what we can do to both verify this and make sure someone doesn’t break it by accident in the future.

(In reply to Darin Adler from comment #7) > Comment on attachment 384892 [details] > > Yes, I suppose the main-thread assertion could just be removed for isSupportedImageMIMETypesForEncoding? It doesn't alter anything that I can see... > > Maybe. > > We couldn’t use it exactly as is, because we’d need to use a thread-safe way > of initializing the global. C++ provides thread safety for initializing > globals in general, but the WebKit project uses a compiler option that turns > off that thread safety. Need to use call_once explicitly instead. > > Then we’d need to audit potential thread safety issues in the > HashMap::contains function. If the code does any reference count > incrementing/decrementing at all, even by accident, then it would be a > thread safety problem. Inspection of the reference count is safe, though. I > wonder what we can do to both verify this and make sure someone doesn’t > break it by accident in the future. I've been reading through the HashMap code - HashMap::contains calls HashTable::contains, which calls HashTable::lookup which calls HashTable::inlineLookup. In inlineLookup, there is code that isn't thread-safe inside the DUMP_HASHTABLE_STATS_PER_TABLE blocks. The non-per-table version uses atomic values and a mutex guarding the non-atomic values, but the per-table version does not. I guess this is pretty easily fixed by mirroring the non-per-table code. Otherwise, HashTable::inlineLookup also calls out to: checkKey HashTranslator::equal isEmptyBucket isDeletedBucket checkKey relies on HashTranslator::equal but is otherwise fine. isEmptyBucket calls isHashTraitsEmptyValue, which appears safe (I have to admit, it got pretty gnarly for me to fully understand this part at a glance, but it looks like it's essentially just doing comparisons with constant values for the most part - though it would depend on the specified or derived Traits template parameter). isDeletedBucket calls KeyTraits::isDeletedValue, which is much like the above, and should be safe (depending on the implementation of HashTraits). No ref-manipulation is involved in the process that I can see, and the only unsafe part is the DUMP_HASHTABLE_STATS_PER_TABLE as mentioned above. So contains/lookup look safe to use within a thread to me, assuming that the contents of the HashTable are guaranteed not to be changed during the lookup, and assuming that the implementation of HashTraits is also thread-safe (which all of the default implementations look to be). I can't immediately think of how we could verify this beyond auditing the code like I've done above (and I'm sure there are people far more qualified than me to do it) and how to keep it thread-safe, assuming it is (which it appears to be, with that one exception). Perhaps a version of HashSet, or a parameter that lets you disable any modification after initialisation? An alternative to this I suppose would be to use per-thread data to create/store an image MIME-types HashSet per thread, rather than using a static variable. Then we wouldn't need to worry about thread safety at all. Any thoughts?

(In reply to Chris Lord from comment #6) > (In reply to Darin Adler from comment #5) > > > Source/WebCore/html/OffscreenCanvas.cpp:196 > > > + promise->reject(InvalidStateError); > > > > I don’t see a test for this case. > > I believe this is tested in > imported/w3c/web-platform-tests/offscreen-canvas/convert-to-blob/ > offscreencanvas.convert.to.blob(.w).html Following up on this, this still fails because the test relies on being able to transfer the OffscreenCanvas, which is bug 202574.

(In reply to Chris Lord from comment #6) > (In reply to Darin Adler from comment #5) > > > Source/WebCore/html/OffscreenCanvas.cpp:188 > > > + promise->reject(SecurityError); > > > > I don’t see a test for this case. > > I think this is missing, I'll need to follow up in wpt. Of course, after writing the test I find this is actually explicitly tested in imagebitmap-renderingcontext/toBlob-origin-clean-offscreen.sub.html so it's just the EncodingError that isn't tested. I've written tests for that and will submit them to wpt.

(In reply to Chris Lord from comment #10) > Of course, after writing the test I find this is actually explicitly tested > in imagebitmap-renderingcontext/toBlob-origin-clean-offscreen.sub.html so > it's just the EncodingError that isn't tested. I've written tests for that > and will submit them to wpt. One parting shot on this point: If we want to be super-thorough (perhaps excessively so), we’d also make sure we have tests that check which of the exceptions is the one generated when there are multiple problems for the same call to make sure the precedence is correct. This kind of thing has been the source of WPT test failures in WebKit before and I suspect it has possibly even been the source of practical website incompatibilities in the past.

Comment on attachment 384892 [details] Patch I’m tempted to mark this review+ but I think there is a new patch coming that addresses some of the comments?

(In reply to Darin Adler from comment #12) > Comment on attachment 384892 [details] > Patch > > I’m tempted to mark this review+ but I think there is a new patch coming > that addresses some of the comments? Yes, I'd hold off, one of the missing tests did actually expose a tiny bit of missing functionality (about 4 lines worth, but I'd still prefer to have it reviewed) - the question of the best way to resolve the SupportedImageMIMETypesForEncoding issue is also still open. Do we just remove the main-thread assert, or do we use per-thread data and create a new hash per thread (which is perhaps excessive, but perhaps more robust)? Or some other solution?

(In reply to Chris Lord from comment #13) > Do we just remove the main-thread assert, or do we use per-thread data and > create a new hash per thread (which is perhaps excessive, but perhaps more > robust)? Or some other solution? I’m torn. I suppose I’d lean towards per-thread data, rather than creating a new one every time the function is called. I’d love to eventually share only one global one across threads but I don’t see a way to do this easily that’s maintainably safe.

(In reply to Darin Adler from comment #14) > (In reply to Chris Lord from comment #13) > > Do we just remove the main-thread assert, or do we use per-thread data and > > create a new hash per thread (which is perhaps excessive, but perhaps more > > robust)? Or some other solution? > > I’m torn. I suppose I’d lean towards per-thread data, rather than creating a > new one every time the function is called. I’d love to eventually share only > one global one across threads but I don’t see a way to do this easily that’s > maintainably safe. I agree, I'll do this. Re EncodingError tests, I realised afterwards (and really, I should've done the necessary reading beforehand...) that there's no way to trigger this from JS. Specifying an unsupported/invalid mimetype uses image/png (by spec), this only happens if there's some unspecified error in creating the image. I did, however, discover that the worker exception tests are broken (they would always pass), so I've fixed those and will include that fix. I've opened a corresponding issue/pull-request on wpt.

Created attachment 385246 [details] Patch

Just a note, there's a new failure in the worker tests for convertToBlob - this is correct and the previous pass was wrong because of https://github.com/web-platform-tests/wpt/issues/20694 - this test will be fixed by bug 202574. Although SecurityError is checked by a test, it's not a test we've imported and it relies on unimplemented functionality, so it does make sense to add SecurityError tests - I've done so, and the corresponding wpt bug: https://github.com/web-platform-tests/wpt/issues/20698

Created attachment 385248 [details] Patch

Comment on attachment 385248 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385248&action=review Looks great, but I’m not 100% happy with every detail. > Source/WebCore/html/OffscreenCanvas.cpp:212 > + promise->resolveWithNewlyCreated<IDLInterface<Blob>>(blob); What about the WTFMove I mentioned last time? Would it not save one little reference count churn here? > Source/WebCore/html/OffscreenCanvas.cpp:222 > + auto* context = canvasBaseScriptExecutionContext(); If context is guaranteed to be non-null, then I suggest making the local variable be a reference rather than a pointer, and dereferencing here. > Source/WebCore/html/OffscreenCanvas.cpp:223 > + if (context->isWorkerGlobalScope()) When paired with downcast, I prefer to use is<>, the two function templates are designed to be used together, so this should be is<WorkerGlobalScope>(*context) instead. > Source/WebCore/html/OffscreenCanvas.cpp:226 > + ASSERT(context->isDocument()); This assertion isn’t valuable. The point of downcast<Document> is exactly that, includes the assertion plus a static_cast. So no need to also write a separate assertion. > Source/WebCore/platform/ThreadGlobalData.cpp:127 > +static HashSet<String, ASCIICaseInsensitiveHash> createSupportedImageMIMETypesForEncoding() Seems inelegant that this entire function ends up here in a file that was formerly not really filled with specific code. Instead we should find a way to leave this code in MIMETypeRegistry.cpp. Something like this in MIMETypeRegistry.h perhaps: std::unique_ptr<MIMETypeRegistryThreadGlobalData> createMIMETypeRegistryThreadGlobalData(); Not sure all the details but generally it would be strange to have all this code in the ThreadGlobalData source file. And this way we'd have less specifics in the ThreadGlobalData header too, just the name MIMETypeRegistryThreadGlobalData and not the specifics of exactly what kind of HashSet.

Created attachment 385382 [details] Patch

All comments addressed, I believe - sorry for the WTFMove one, I just missed that in amongst the other bits.

Comment on attachment 385382 [details] Patch Clearing flags on attachment: 385382 Committed r253474: <https://trac.webkit.org/changeset/253474>

All reviewed patches have been landed. Closing bug.

<rdar://problem/57912779>