Bug 236069

Summary: REGRESSION (iOS 15.1 / r280824) QuickLook - model not loading when passing extra parameters
Product: WebKit Reporter: Maciek Główka <maciek.glowka>
Component: New BugsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, cdumez, darin, dino, ggaren, graouts, mail, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Safari 15   
Hardware: iPhone / iPad   
OS: iOS 15   
See Also: https://bugs.webkit.org/show_bug.cgi?id=226265
https://bugs.webkit.org/show_bug.cgi?id=228923
https://bugs.webkit.org/show_bug.cgi?id=238221
Attachments:
Description Flags
WIP Patch (needs test)
none
Patch none

Maciek Główka
Reported 2022-02-03 02:12:22 PST
On iOS 15.2 when passing USDZ from Safari to QuickLook AR, as a blob with parameters (eg. fixed scaling), the model is never rendered. The loader spinner keeps spinning infinitely. You can repro the bug here: http://tests.glowka.atthost24.pl/ Built using model-viewer.dev component. USDZ is generated on the fly. I believe model-viewer's 'fixed scaling' is translated as #allowsContentScaling=0 Without the parameter models loads as expected. Possibly related to: https://bugs.webkit.org/show_bug.cgi?id=226265
Attachments
WIP Patch (needs test) (2.86 KB, patch)
2022-03-22 09:41 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (9.47 KB, patch)
2022-03-22 10:20 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2022-02-03 16:18:19 PST
<rdar://problem/88461772>
Antoine Quint
Comment 2 2022-03-21 07:46:33 PDT
We get this error in [_WKPreviewControllerDataSource failWithError]. Domain=WebKitBlobResource Code=1 "(null)" UserInfo={NSErrorFailingURLKey=blob:http://tests.glowka.atthost24.pl/24701d34-f863-4935-af4e-8b13c1ee9492#allowsContentScaling=0, NSErrorFailingURLStringKey=blob:http://tests.glowka.atthost24.pl/24701d34-f863-4935-af4e-8b13c1ee9492#allowsContentScaling=0}
Antoine Quint
Comment 3 2022-03-22 02:49:01 PDT
This regressed with r280824, the fix for bug 228923.
Antoine Quint
Comment 4 2022-03-22 02:58:49 PDT
To whomever may work on this: this type of content leads to SystemPreviewControllerCocoa trying to open a blob URL for a failed download.
Chris Dumez
Comment 5 2022-03-22 09:26:00 PDT
I know what's going on and I am validating a fix.
Chris Dumez
Comment 6 2022-03-22 09:41:03 PDT
Created attachment 455385 [details] WIP Patch (needs test)
Chris Dumez
Comment 7 2022-03-22 10:20:08 PDT
Created attachment 455388 [details] Patch
Maciek Główka
Comment 8 2022-03-22 10:37:46 PDT
Thanks for taking care of the bug! Will the patch push the fixed-scale parameter to QuickLook as well or is it only model-not-loading-at-all fix ? Regards, Maciek
Chris Dumez
Comment 9 2022-03-22 10:40:06 PDT
(In reply to Maciek Główka from comment #8) > Thanks for taking care of the bug! > > Will the patch push the fixed-scale parameter to QuickLook as well or is it > only model-not-loading-at-all fix ? > > Regards, > Maciek I know very little about USDZ but I am keeping the fragment as part of the blob URL (just making the blob download actually succeed if its URL contains a fragment) so I would expect it to be passed to QuickLook. If you tell me exactly how to validate though, I am happy to do so.
Maciek Główka
Comment 10 2022-03-22 10:59:43 PDT
Thanks. I guess if the #allowsContentScaling=0 is passed it should be sufficient. As far as I know when .usdz file is given with the fragment it's working as expected. Only the blobs were problematic.
Darin Adler
Comment 11 2022-03-22 11:58:15 PDT
Comment on attachment 455388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455388&action=review > Source/WebCore/platform/network/BlobRegistryImpl.cpp:56 > +static String blobURLWithoutFragment(const URL& url) > +{ > + return url.hasFragmentIdentifier() ? url.stringWithoutFragmentIdentifier().toString() : url.string(); > +} This should probably eventually be a URL member function rather than a blob-specific helper, even if not in this patch. Generally things that return StringView but we want to optimize the "unchanged pre-existing String" case are a recurring pattern. Just need the right name, I suppose. Maybe we need to rename the existing function to viewWithoutFragmentIdentifier and this operation can be stringWithoutFragmentIdentifier, but I hate to have this here.
Chris Dumez
Comment 12 2022-03-22 12:04:35 PDT
Comment on attachment 455388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455388&action=review >> Source/WebCore/platform/network/BlobRegistryImpl.cpp:56 >> +} > > This should probably eventually be a URL member function rather than a blob-specific helper, even if not in this patch. Generally things that return StringView but we want to optimize the "unchanged pre-existing String" case are a recurring pattern. Just need the right name, I suppose. Maybe we need to rename the existing function to viewWithoutFragmentIdentifier and this operation can be stringWithoutFragmentIdentifier, but I hate to have this here. I like the idea. I'll implement this in a follow-up though to facilitate cherry-picking of this patch to branches.
EWS
Comment 13 2022-03-22 12:53:05 PDT
Committed r291689 (248731@main): <https://commits.webkit.org/248731@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 455388 [details].
Miles
Comment 14 2022-04-12 10:31:02 PDT
This issue still exists in iOS 15.5 beta (19F5047e)
Antoine Quint
Comment 15 2022-04-12 12:46:42 PDT
(In reply to Miles from comment #14) > This issue still exists in iOS 15.5 beta (19F5047e) Yes, the code change is not in that beta release. I can't comment on when it will be available just yet, but that is expected.
Miles
Comment 16 2022-04-15 14:22:00 PDT
Thanks for the update. I'll keep watching in anticipation. Cheers
Miles
Comment 17 2022-04-25 09:00:21 PDT
Just FYI... This issue still exists in iOS 15.5 beta 2 (19F5057e) I'll retest with the next update. Thanks.
Miles
Comment 18 2022-04-28 03:01:54 PDT
Issue still exists in iOS 15.5 beta 3 (19F5062g) I live in hope for the next update ;)
Miles
Comment 19 2022-05-05 01:46:42 PDT
This looks to be resolved in iOS 15.5 beta 4 (19F5070b). So far I have tested opening USDZ as blob in AR QuickLook along with #allowsContentScaling=0 parameter, and the model does now load and QuickLook does indeed correctly prevent user from scaling the 3D model. Great!
Brent Fulgham
Comment 20 2022-05-26 14:53:58 PDT
This fix shipped with Safari 15.5 (all platforms).
Note You need to log in before you can comment on or make changes to this bug.