Bug 217315 - Lazy loaded iframe should not lazy load when scripting is disabled
Summary: Lazy loaded iframe should not lazy load when scripting is disabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-05 08:28 PDT by Rob Buis
Modified: 2020-12-01 09:14 PST (History)
10 users (show)

See Also:


Attachments
Patch (5.52 KB, patch)
2020-10-05 08:29 PDT, Rob Buis
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (5.52 KB, patch)
2020-10-05 08:52 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.43 KB, patch)
2020-10-16 01:11 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (4.84 KB, patch)
2020-11-27 08:32 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (5.50 KB, patch)
2020-11-29 12:13 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (8.49 KB, patch)
2020-11-30 05:40 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (5.35 KB, patch)
2020-12-01 02:32 PST, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2020-10-05 08:28:02 PDT
Lazy loaded iframe should not lazy load when scripting is disabled [1].

[1] https://html.spec.whatwg.org/#will-lazy-load-element-steps
Comment 1 Rob Buis 2020-10-05 08:29:57 PDT
Created attachment 410519 [details]
Patch
Comment 2 EWS Watchlist 2020-10-05 08:30:34 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 3 Rob Buis 2020-10-05 08:52:23 PDT
Created attachment 410522 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2020-10-12 08:28:16 PDT
<rdar://problem/70207607>
Comment 5 Rob Buis 2020-10-16 01:11:56 PDT
Created attachment 411543 [details]
Patch
Comment 6 Rob Buis 2020-11-27 08:32:52 PST
Created attachment 414945 [details]
Patch
Comment 7 Sam Weinig 2020-11-28 11:53:32 PST
Comment on attachment 414945 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414945&action=review

> Source/WebCore/html/HTMLIFrameElement.cpp:173
>  
> +    if (document.frame() && !document.frame()->script().canExecuteScripts(NotAboutToExecuteScript))
> +        return false;

For a document without a frame (e.g. one created via new Document() in JS, it seems like this should probably also return false, since script is pretty much implicitly disabled there. 

Would be nice to test that case as well (if its meaningful).
Comment 8 Rob Buis 2020-11-29 12:13:57 PST
Created attachment 414990 [details]
Patch
Comment 9 Rob Buis 2020-11-29 13:33:01 PST
Comment on attachment 414945 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414945&action=review

>> Source/WebCore/html/HTMLIFrameElement.cpp:173
>> +        return false;
> 
> For a document without a frame (e.g. one created via new Document() in JS, it seems like this should probably also return false, since script is pretty much implicitly disabled there. 
> 
> Would be nice to test that case as well (if its meaningful).

Very good point, thanks! Fixed.
Comment 10 Darin Adler 2020-11-29 21:20:44 PST
Comment on attachment 414990 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414990&action=review

> Source/WebCore/html/HTMLIFrameElement.cpp:172
> +    if (!document.frame() || !document.frame()->script().canExecuteScripts(NotAboutToExecuteScript))

Can we find a way to share this expression with HTMLImageElement::isLazyLoadable? Null check on frame makes this expression long enough that it seems worth while. Maybe.
Comment 11 Rob Buis 2020-11-30 05:40:57 PST
Created attachment 415018 [details]
Patch
Comment 12 Rob Buis 2020-11-30 07:42:39 PST
Comment on attachment 414990 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414990&action=review

>> Source/WebCore/html/HTMLIFrameElement.cpp:172
>> +    if (!document.frame() || !document.frame()->script().canExecuteScripts(NotAboutToExecuteScript))
> 
> Can we find a way to share this expression with HTMLImageElement::isLazyLoadable? Null check on frame makes this expression long enough that it seems worth while. Maybe.

The best way to share, I think, is to make it even more general, since this construction is used in a few places in HTML. So I added a helper to HTMLElement.
Comment 13 Rob Buis 2020-12-01 02:32:37 PST
Created attachment 415118 [details]
Patch
Comment 14 EWS 2020-12-01 09:14:53 PST
Committed r270300: <https://trac.webkit.org/changeset/270300>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415118 [details].