Bug 236069 - REGRESSION (iOS 15.1 / r280824) QuickLook - model not loading when passing extra parameters
Summary: REGRESSION (iOS 15.1 / r280824) QuickLook - model not loading when passing ex...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Safari 15
Hardware: iPhone / iPad iOS 15
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-03 02:12 PST by Maciek Główka
Modified: 2022-05-26 14:53 PDT (History)
10 users (show)

See Also:


Attachments
WIP Patch (needs test) (2.86 KB, patch)
2022-03-22 09:41 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (9.47 KB, patch)
2022-03-22 10:20 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maciek Główka 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
Comment 1 Radar WebKit Bug Importer 2022-02-03 16:18:19 PST
<rdar://problem/88461772>
Comment 2 Antoine Quint 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}
Comment 3 Antoine Quint 2022-03-22 02:49:01 PDT
This regressed with r280824, the fix for bug 228923.
Comment 4 Antoine Quint 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.
Comment 5 Chris Dumez 2022-03-22 09:26:00 PDT
I know what's going on and I am validating a fix.
Comment 6 Chris Dumez 2022-03-22 09:41:03 PDT
Created attachment 455385 [details]
WIP Patch (needs test)
Comment 7 Chris Dumez 2022-03-22 10:20:08 PDT
Created attachment 455388 [details]
Patch
Comment 8 Maciek Główka 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
Comment 9 Chris Dumez 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.
Comment 10 Maciek Główka 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.
Comment 11 Darin Adler 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.
Comment 12 Chris Dumez 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.
Comment 13 EWS 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].
Comment 14 Miles 2022-04-12 10:31:02 PDT
This issue still exists in iOS 15.5 beta (19F5047e)
Comment 15 Antoine Quint 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.
Comment 16 Miles 2022-04-15 14:22:00 PDT
Thanks for the update. 
I'll keep watching in anticipation. 
Cheers
Comment 17 Miles 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.
Comment 18 Miles 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 ;)
Comment 19 Miles 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!
Comment 20 Brent Fulgham 2022-05-26 14:53:58 PDT
This fix shipped with Safari 15.5 (all platforms).