Bug 240120 – Image controls menu button is not appearing for multi-page PDFs

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Kate Cheney]

Image controls menu button is not appearing for multi-page PDFs

[Kate Cheney]

Created attachment 458873 [details]
Patch

[Kate Cheney]

rdar://86425721

[Wenson Hsieh]

Comment on attachment 458873 [details]
Patch

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

> Source/WebCore/html/HTMLElement.cpp:1123
> +void HTMLElement::updateImageControls()

Nit - I think it might be more ideal to move these helpers into ImageControlsMac.(h/cpp) as helper functions that take in an `HTMLElement&`, instead of adding these as methods directly to HTMLElement.

> Source/WebCore/html/HTMLElement.cpp:1132
> +    document().eventLoop().queueTask(TaskSource::InternalAsyncTask, [this, protectedThis = Ref { *this }] {

Capturing `this` by raw pointer here seems a bit unsafe — I think it's possible for this to result in a UAF if the element is destroyed after the event loop task is scheduled, but before it is dispatched.

> Source/WebCore/html/HTMLElement.h:194
> +    bool m_imageMenuEnabled { false };

Does this make HTMLElement bigger? If so, we _may_ want to consider an approach where we maintain a WeakSet<HTMLElement> on something like Page instead, which would keep track of image-menu-enabled elements. In practice, this would always be empty, except for in Mail on macOS.

[Kate Cheney]

Comment on attachment 458873 [details]
Patch

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

>> Source/WebCore/html/HTMLElement.cpp:1123
>> +void HTMLElement::updateImageControls()
> 
> Nit - I think it might be more ideal to move these helpers into ImageControlsMac.(h/cpp) as helper functions that take in an `HTMLElement&`, instead of adding these as methods directly to HTMLElement.

Good idea.

>> Source/WebCore/html/HTMLElement.cpp:1132
>> +    document().eventLoop().queueTask(TaskSource::InternalAsyncTask, [this, protectedThis = Ref { *this }] {
> 
> Capturing `this` by raw pointer here seems a bit unsafe — I think it's possible for this to result in a UAF if the element is destroyed after the event loop task is scheduled, but before it is dispatched.

Good point. I am curious why this is async in the first place, I copied it over from existing code. Maybe Megan remembers why?

>> Source/WebCore/html/HTMLElement.h:194
>> +    bool m_imageMenuEnabled { false };
> 
> Does this make HTMLElement bigger? If so, we _may_ want to consider an approach where we maintain a WeakSet<HTMLElement> on something like Page instead, which would keep track of image-menu-enabled elements. In practice, this would always be empty, except for in Mail on macOS.

Yeah ok let me see if I can figure out something different.

[Wenson Hsieh]

> >> Source/WebCore/html/HTMLElement.h:194
> >> +    bool m_imageMenuEnabled { false };
> > 
> > Does this make HTMLElement bigger? If so, we _may_ want to consider an approach where we maintain a WeakSet<HTMLElement> on something like Page instead, which would keep track of image-menu-enabled elements. In practice, this would always be empty, except for in Mail on macOS.
> 
> Yeah ok let me see if I can figure out something different.

Ah, so I just did a bit of testing — this new `bool` flag won't make HTMLElement any bigger, it seems, since it fits in the padding space after `StyledElement`:

(Before)
```
  +0 <120> WebCore::HTMLElement
  +0 <120>     WebCore::StyledElement WebCore::StyledElement

    …

+112 <  1>           bool m_hasDuplicateAttribute
+113 <  7>   <PADDING: 7 bytes>
Total byte size: 120
Total pad bytes: 7
```

(After)
```
  +0 <120> WebCore::HTMLElement
  +0 <120>     WebCore::StyledElement WebCore::StyledElement

    …

+112 <  1>           bool m_hasDuplicateAttribute
+113 <  1>   bool m_testing
+114 <  6>   <PADDING: 6 bytes>
Total byte size: 120
Total pad bytes: 6
Padding percentage: 5.00 %
```

...so I think adding the new bool will be okay for now, at least w.r.t. perf/memory impact.

[Kate Cheney]

(In reply to Wenson Hsieh from comment #5)
> > >> Source/WebCore/html/HTMLElement.h:194
> > >> +    bool m_imageMenuEnabled { false };
> > > 
> > > Does this make HTMLElement bigger? If so, we _may_ want to consider an approach where we maintain a WeakSet<HTMLElement> on something like Page instead, which would keep track of image-menu-enabled elements. In practice, this would always be empty, except for in Mail on macOS.
> > 
> > Yeah ok let me see if I can figure out something different.
> 
> Ah, so I just did a bit of testing — this new `bool` flag won't make
> HTMLElement any bigger, it seems, since it fits in the padding space after
> `StyledElement`:
> 
> (Before)
> ```
>   +0 <120> WebCore::HTMLElement
>   +0 <120>     WebCore::StyledElement WebCore::StyledElement
> 
>     …
> 
> +112 <  1>           bool m_hasDuplicateAttribute
> +113 <  7>   <PADDING: 7 bytes>
> Total byte size: 120
> Total pad bytes: 7
> ```
> 
> (After)
> ```
>   +0 <120> WebCore::HTMLElement
>   +0 <120>     WebCore::StyledElement WebCore::StyledElement
> 
>     …
> 
> +112 <  1>           bool m_hasDuplicateAttribute
> +113 <  1>   bool m_testing
> +114 <  6>   <PADDING: 6 bytes>
> Total byte size: 120
> Total pad bytes: 6
> Padding percentage: 5.00 %
> ```
> 
> ...so I think adding the new bool will be okay for now, at least w.r.t.
> perf/memory impact.

Thanks for testing that! It also isn't too messy to just store the bool in the necessary elements and downcast the HTMLElement to access it. Since the majority of HTMLElements won't need this at all, it may be the best long term solution to avoid any future perf impact.

[Wenson Hsieh]

> > ...so I think adding the new bool will be okay for now, at least w.r.t.
> > perf/memory impact.
> 
> Thanks for testing that! It also isn't too messy to just store the bool in
> the necessary elements and downcast the HTMLElement to access it. Since the
> majority of HTMLElements won't need this at all, it may be the best long
> term solution to avoid any future perf impact.

That sounds good to me! (i.e. just keeping the flag that's already on HTMLImageElement, and adding a similar bool to HTMLAttachmentElement).

[Kate Cheney]

Created attachment 458914 [details]
Patch

[Kate Cheney]

(In reply to Kate Cheney from comment #8)
> Created attachment 458914 [details]
> Patch

Ugh, need to rebase

[Kate Cheney]

Created attachment 458918 [details]
Patch

[Aditya Keerthi]

Comment on attachment 458918 [details]
Patch

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

> Source/WebCore/dom/mac/ImageControlsMac.cpp:157
> +        if (is<HTMLImageElement>(node.shadowHost())) {

This could be written as:

```
if (RefPtr imageElement = dynamicDowncast<HTMLImageElement>(node.shadowHost())) {

} else (RefPtr attachmentElement = dynamicDowncast<HTMLAttachmentElement>(node.shadowHost())) {

}
```

Right now, it performs redundant `is` checks, due to the use of `dynamicDowncast`.

> Source/WebCore/dom/mac/ImageControlsMac.cpp:198
> +        if (is<HTMLAttachmentElement>(protectedElement))

Ditto (ImageControlsMac.cpp:157).

> Source/WebCore/dom/mac/ImageControlsMac.cpp:231
> +    if (is<RenderImage>(*renderObject))

This doesn't have the same redundant `is` check due to the use of `downcast`, but maybe this should also be written using `dynamicDowncast` and a variable to match the proposed style above?

> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:221
> +        isPDFAttachment = attachment->utiType() == [UTTypePDF.identifier UTF8String];

Is it safe to use `UTType` yet? UniformTypeIdentifiers.framework is only available on macOS 11.0+.

[Kate Cheney]

(In reply to Aditya Keerthi from comment #11)
> Comment on attachment 458918 [details]
> Patch
> 

Thanks for looking!

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458918&action=review
> 
> > Source/WebCore/dom/mac/ImageControlsMac.cpp:157
> > +        if (is<HTMLImageElement>(node.shadowHost())) {
> 
> This could be written as:
> 
> ```
> if (RefPtr imageElement =
> dynamicDowncast<HTMLImageElement>(node.shadowHost())) {
> 
> } else (RefPtr attachmentElement =
> dynamicDowncast<HTMLAttachmentElement>(node.shadowHost())) {
> 
> }
> ```
> 
> Right now, it performs redundant `is` checks, due to the use of
> `dynamicDowncast`.
> 
> > Source/WebCore/dom/mac/ImageControlsMac.cpp:198
> > +        if (is<HTMLAttachmentElement>(protectedElement))
> 
> Ditto (ImageControlsMac.cpp:157).
> 
> > Source/WebCore/dom/mac/ImageControlsMac.cpp:231
> > +    if (is<RenderImage>(*renderObject))
> 
> This doesn't have the same redundant `is` check due to the use of
> `downcast`, but maybe this should also be written using `dynamicDowncast`
> and a variable to match the proposed style above?
> 

Yes, will fix all of these.

> > Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:221
> > +        isPDFAttachment = attachment->utiType() == [UTTypePDF.identifier UTF8String];
> 
> Is it safe to use `UTType` yet? UniformTypeIdentifiers.framework is only
> available on macOS 11.0+.

Hmm good call, I'll add some compile time flags here and use the deprecated version for older OS's.

[Chris Dumez]

Comment on attachment 458918 [details]
Patch

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

> Source/WebCore/dom/mac/ImageControlsMac.cpp:158
> +            RefPtr shadowHost = dynamicDowncast<HTMLImageElement>(node.shadowHost());

You already did a is<HTMLImageElement>() so you shouldn't use a dynamicDowncast<> here, just a downcast<>(). No point in checking the type twice. But really, I would just change the if condition to:
```
if (RefPtr shadowHost = dynamicDowncast<HTMLImageElement>(node.shadowHost())) {
```

> Source/WebCore/dom/mac/ImageControlsMac.cpp:159
> +            if (!shadowHost)

Again, this check is redundant because of your if check above. We know shadowHost() is a non-null HTMLImageElement.

> Source/WebCore/dom/mac/ImageControlsMac.cpp:168
> +            RefPtr shadowHost = dynamicDowncast<HTMLAttachmentElement>(node.shadowHost());

Same comments as above.

> Source/WebCore/dom/mac/ImageControlsMac.cpp:195
> +        ASSERT(is<HTMLAttachmentElement>(protectedElement) || is<HTMLImageElement>(protectedElement));

ASSERT(is<HTMLAttachmentElement>(*protectedElement) || is<HTMLImageElement>(*protectedElement));

to avoid unnecessary null checks.

> Source/WebCore/dom/mac/ImageControlsMac.cpp:199
> +            hasImageMenu = dynamicDowncast<HTMLAttachmentElement>(*protectedElement)->imageMenuEnabled();

downcast<>, not dynamicDowncast<>. You're making me wonder if introducing dynamicDowncast<> was a good decision :)

> Source/WebCore/dom/mac/ImageControlsMac.cpp:201
> +            hasImageMenu = dynamicDowncast<HTMLImageElement>(*protectedElement)->imageMenuEnabled();

ditto.

> Source/WebCore/dom/mac/ImageControlsMac.cpp:223
> +    if (RefPtr<Node> node = shadowRoot->firstChild()) {

if (RefPtr node = shadowRoot->firstChild()) {

should work

> Source/WebCore/dom/mac/ImageControlsMac.cpp:224
> +        RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(ImageControlsMac::hasControls(downcast<HTMLElement>(*node)));

A little unclear why it is guaranteed that node is an HTMLElement.

> Source/WebCore/html/HTMLAttachmentElement.h:81
> +    bool imageMenuEnabled() const { return m_imageMenuEnabled; }

missing prefix (probably "is" here) for boolean variable / getter.

> Source/WebCore/html/HTMLAttachmentElement.h:101
> +    bool childShouldCreateRenderer(const Node&) const override;

Can this be final?

> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:219
> +    auto attachment = page()->attachmentForIdentifier(m_context.controlledImageAttachmentID());

If not used outside the if scope, I suggest:
if (auto attachment = page()->attachmentForIdentifier(m_context.controlledImageAttachmentID()))

[Kate Cheney]

Comment on attachment 458918 [details]
Patch

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

>> Source/WebCore/dom/mac/ImageControlsMac.cpp:199
>> +            hasImageMenu = dynamicDowncast<HTMLAttachmentElement>(*protectedElement)->imageMenuEnabled();
> 
> downcast<>, not dynamicDowncast<>. You're making me wonder if introducing dynamicDowncast<> was a good decision :)

honest mistake!

>> Source/WebCore/dom/mac/ImageControlsMac.cpp:224
>> +        RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(ImageControlsMac::hasControls(downcast<HTMLElement>(*node)));
> 
> A little unclear why it is guaranteed that node is an HTMLElement.

Good point, I am not sure how that assumption was made in the original patch. Will ask Megan.

>> Source/WebCore/html/HTMLAttachmentElement.h:81
>> +    bool imageMenuEnabled() const { return m_imageMenuEnabled; }
> 
> missing prefix (probably "is" here) for boolean variable / getter.

Will fix.

>> Source/WebCore/html/HTMLAttachmentElement.h:101
>> +    bool childShouldCreateRenderer(const Node&) const override;
> 
> Can this be final?

seems so, compiler will tell me for sure!

>> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:219
>> +    auto attachment = page()->attachmentForIdentifier(m_context.controlledImageAttachmentID());
> 
> If not used outside the if scope, I suggest:
> if (auto attachment = page()->attachmentForIdentifier(m_context.controlledImageAttachmentID()))

I use it below when creating the itemProvider, did this to avoid duplicating.

Thanks for comments!

[Kate Cheney]

Created attachment 458955 [details]
Patch

[Kate Cheney]

Created attachment 458958 [details]
Patch

[Megan Gardner]

Comment on attachment 458958 [details]
Patch

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

> Source/WebCore/dom/mac/ImageControlsMac.cpp:195
> +

nit. I don't think you need this space.

> Source/WebCore/dom/mac/ImageControlsMac.cpp:197
> +

nit. I don't think you need this space.

> Source/WebCore/dom/mac/ImageControlsMac.cpp:198
> +        bool hasImageMenu = isImageMenuEnabled(*protectedElement);

I feel like hasImgeMenu would be better named isImageMenuEnabled, both the hasImageMenu and hasControls sound like they should be the same thing, but the difference really is if they are enable or not.

> Source/WebCore/html/HTMLAttachmentElement.h:101
> +    bool childShouldCreateRenderer(const Node&) const final;

should this be const override?

[Kate Cheney]

Comment on attachment 458958 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/dom/mac/ImageControlsMac.cpp:195
>> +
> 
> nit. I don't think you need this space.

will remove.

>> Source/WebCore/dom/mac/ImageControlsMac.cpp:197
>> +
> 
> nit. I don't think you need this space.

ditto!

>> Source/WebCore/dom/mac/ImageControlsMac.cpp:198
>> +        bool hasImageMenu = isImageMenuEnabled(*protectedElement);
> 
> I feel like hasImgeMenu would be better named isImageMenuEnabled, both the hasImageMenu and hasControls sound like they should be the same thing, but the difference really is if they are enable or not.

Good point. I can't use the exact same name as the function because the C++ compiler gets confused, but I can do something similar like "imageMenuEnabled". 

>> Source/WebCore/html/HTMLAttachmentElement.h:101
>> +    bool childShouldCreateRenderer(const Node&) const final;
> 
> should this be const override?

I think final is OK because no derived class will override it, see Chris's comment above.

[Kate Cheney]

Created attachment 459049 [details]
Patch for landing

[EWS]

Committed r293977 (250415@main): <https://commits.webkit.org/250415@main>

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