Bug 136269 – Drawing text in an SVG font causes load events to be fired

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Drawing text in an SVG font causes load events to be fired

[Myles C. Maxfield]

Created attachment 237184 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/15724915>

[Darin Adler]

Comment on attachment 237184 [details]
Patch

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

> Source/WebCore/loader/cache/CachedFont.cpp:154
> -        m_externalSVGDocument->setContent(decoder->decodeAndFlush(m_data->data(), m_data->size()));
> +        const auto& content = decoder->decodeAndFlush(m_data->data(), m_data->size());
> +        m_externalSVGDocument->open();
> +        if (auto* parser = m_externalSVGDocument->parser()) {
> +            parser->append(content.impl());
> +            parser->finish();
> +            parser->detach();
> +        }
> +#if !ASSERT_DISABLED
> +        else
> +            ASSERT_NOT_REACHED();
> +#endif
> +        // Don't call m_externalSVGDocument->close() because that might fire load events. https://bugs.webkit.org/show_bug.cgi?id=14568

If we really need to do this, then how about just making a helper function called something like setContentWithLoadEventWorkaround? I don’t see any reason to put all this code in line here just so we can remove it later once we fix that bug.

[Myles C. Maxfield]

Created attachment 237215 [details]
Patch

[Myles C. Maxfield]

Created attachment 237229 [details]
Patch

[Darin Adler]

Comment on attachment 237215 [details]
Patch

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

review- because this breaks the build

I think there is a question about the best way to accomplish this. I think we should consider the version where we don’t change the data structures quite as much and just refine them so it’s efficient to dispatch events only for the current document, without necessarily allocating separate event sender objects for each document. But there are other possibilities as well.

> Source/WebCore/dom/Document.cpp:2395
> -    ImageLoader::dispatchPendingBeforeLoadEvents();
> -    ImageLoader::dispatchPendingLoadEvents();
> -    ImageLoader::dispatchPendingErrorEvents();
> +    beforeLoadImageEventSender().dispatchPendingEvents();
> +    loadImageEventSender().dispatchPendingEvents();
> +    errorImageEventSender().dispatchPendingEvents();
>  
> -    HTMLLinkElement::dispatchPendingLoadEvents();
> -    HTMLStyleElement::dispatchPendingLoadEvents();
> +    linkLoadEventSender().dispatchPendingEvents();
> +    styleLoadEventSender().dispatchPendingEvents();

I think using the data members directly might be better than using these accessors. Longer term I am not sure we want to expose these event sender objects publicly.

Not new to this patch: It seems a little strange that these are dispatched by type rather than “in the order they came in”. Might be something we want to revise later. I could see us making this a single queue of functions rather than a more complex set of separate queues. Building this out of a more generic “queue of functions that can be removed” might be better than building it specifically as a pending event system.

> Source/WebCore/dom/Document.h:37
> +#include "EventSender.h"

This is unfortunate. Adding this include means that EventSender.h needs to be turned into a “private” header, visible to WebKit, rather than a “project” header, inside WebCore only. That is why the build is broken.

The change also could slow down compile times since so many things include Document and they now all would include EventSender. If we can make sure the event senders are all on the heap, then we don’t have to compile in the class here in the Document header. The details might be a bit tricky. We need something stored in a unique_ptr. Maybe a more general purpose private data object. We could forward declare EventSender as long as we didn’t have to compile in the actual data members. Or a DocumentEventSenders object that is declared in a separate header.

Another way to deal with this is issue to make a bigger design change so the document capability this is all built on is a more generic “cancelable functions called at close time” mechanism rather than something that directly knows about event senders. That’s a better long term strategy, but a bigger riskier change to make. The EventSender class would manage these functions. Not clear how to implement the XML “deliver only the before load events” thing with that, though.

Generally speaking it would be nice to minimize how much of this we put into Document.h just to achieve the best possible separation of concerns. It seems the only Document-specific aspect of this is the need to dispatch events inside the close function. Maybe we can do an alternate version of this where everything is done with external hash tables inside EventSender and the only change we make to Document is to pas the document into the dispatchPendingEvents functions in Document::implicitClose.

> Source/WebCore/dom/Document.h:40
> +#include "ImageLoader.h"

More of the same. Sure would be nice to not have to include this header everywhere we include Document.h.

> Source/WebCore/dom/Document.h:169
> +typedef EventSender<ImageLoader> ImageEventSender;
> +typedef EventSender<HTMLLinkElement> LinkEventSender;
> +typedef EventSender<HTMLStyleElement> StyleEventSender;

I’m not sure these typedefs are helpful. I think they slightly obscure the fact that all three types are so closely related and I would prefer we just use the whole type names.

> Source/WebCore/html/HTMLLinkElement.cpp:97
> +LinkEventSender& HTMLLinkElement::linkLoadEventSender()
> +{
> +    return document().linkLoadEventSender();
> +}

Do we really need this helper? Why not do this at each call site explicitly?

> Source/WebCore/html/HTMLStyleElement.cpp:68
> +StyleEventSender& HTMLStyleElement::styleLoadEventSender()
> +{
> +    return document().styleLoadEventSender();
> +}

Do we really need this helper? Why not do this at each call site explicitly?

> Source/WebCore/loader/ImageLoader.cpp:74
> -static ImageEventSender& beforeLoadEventSender()
> +ImageEventSender& ImageLoader::beforeLoadEventSender()
>  {
> -    static NeverDestroyed<ImageEventSender> sender(eventNames().beforeloadEvent);
> -    return sender;
> +    return element().document().beforeLoadImageEventSender();
>  }
>  
> -static ImageEventSender& loadEventSender()
> +ImageEventSender& ImageLoader::loadEventSender()
>  {
> -    static NeverDestroyed<ImageEventSender> sender(eventNames().loadEvent);
> -    return sender;
> +    return element().document().loadImageEventSender();
>  }
>  
> -static ImageEventSender& errorEventSender()
> +ImageEventSender& ImageLoader::errorEventSender()
>  {
> -    static NeverDestroyed<ImageEventSender> sender(eventNames().errorEvent);
> -    return sender;
> +    return element().document().errorImageEventSender();
>  }

Do we really need these helpers? Why not do these at each call site explicitly? Are there a lot of call sites?

> Source/WebCore/loader/ImageLoader.cpp:120
> +    setImageWithoutConsideringPendingLoadEvent(newImage, oldImageDocument ? oldImageDocument : &element().document());

The handling of null here got my attention. There are two different cases here where oldImageDocument could be null.

One case is elementDidMoveToNewDocument. In that case, if the old document is null then we should not pretend there was an old document and supply a non-null values. In such cases we’d not have any events to cancel; we’d want to assert that we don’t. If this object had a pending event, but was not in a document, then it seems likely there is a dangling pointer to this object in some other document.

The other case is for all existing callers of setImage in the cases where we are not moving between documents. In that case, it makes sense that “old” document is the current document.

It’s a little messy to use null for both. I have some thoughts below about how to avoid this.

> Source/WebCore/loader/ImageLoader.cpp:124
>      // Only consider updating the protection ref-count of the Element immediately before returning
>      // from this function as doing so might result in the destruction of this ImageLoader.
>      updatedHasPendingEvent();

I just read the updatedHasPendingEvent function, and it seems that we defer the call to deref using a timer. So these comments about deletion of the image loader are incorrect and obsolete. They should all be removed at some point. For future refactoring, this also means that the reason we have a separate setImageWithoutConsideringPendingLoadEvent function is no obsolete; there is no harm to calling updatedHasPendingEvent inside the function. So that’s a cleanup we should consider at some point.

> Source/WebCore/loader/ImageLoader.cpp:127
> +void ImageLoader::setImageWithoutConsideringPendingLoadEvent(CachedImage* newImage, Document* oldImageDocument)

If after considering my comments we are still never passing null to this function, then I suggest making it take a Document& rather than a Document*.

But I think we may want to pass null when calling this from elementDidMoveToDocument just to assert that we don’t have dangling events. Or we could do that assertion in elementDidMoveToDocument and pass a our document in just so we can take a reference instead of a pointer and don’t have to worry about nulls inside this function.

It’s also bizarre (although not new) that setImage has a side effect of resetting animation even when no image is passed in.

> Source/WebCore/loader/ImageLoader.cpp:144
> +        m_image = newImage;

Why did this line of code need to move? It is important to call cancelEvent before setting m_image? Or is it something we are cleaning up to be more logical?

> Source/WebCore/loader/ImageLoader.cpp:446
> +    setImage(nullptr, oldDocument);

As I mention both above and below, this could instead just say:

    setImageWithoutConsideringPendingLoadEvent(nullptr, oldDocument);
    updatedHasPendingEvent();

This is superior in two ways: 1) Don’t need to add potentially confusing argument to the publicly visible setImage function. 2) Better not to hide the fact that the image loader could be deleted after the setImage function is called; the old code did not make that explicit. It’s funny that every function that calls updatedHasPendingEvent has a long comment explaining the danger of deletion, but this function that was calling setImage had exactly the same fragility, but didn’t comment on it at all.

As a side note, I think that all those three line paragraph versions of updatedHasPendingEvent() in this file should be changed since the comment they contain is inaccurate. And I think we should refactor since that problem has been designed away.

So longer term, this way of dodging the change to setImage won’t really work. I think we can just have a public setImage that does not take a document pointer and a private function that does take a document pointer, possibly named setImage or possibly with another name to emphasize that it’s both for setting a new image and for clearing the image when moving between documents. I’ll note in passing that we never pass non-null values for both the image and the document to the function.

> Source/WebCore/loader/ImageLoader.h:62
> +    void setImage(CachedImage*, Document* oldImageDocument = nullptr); // Cancels pending beforeload and load events, and doesn't dispatch new ones.

Adding this argument to the public function isn’t great. We don’t need to make that capability public. And a default argument of null is also not great. It’s ambiguous whether null means “this wasn’t in a document before” or “I am not passing a document because the document isn’t changing”. We should consider the alternative I mention above.

> Source/WebCore/loader/ImageLoader.h:116
> +    static void checkConsistency(const WebCore::ImageLoader* p);

Should not have the argument name "p" here.

> Source/WebCore/xml/parser/XMLDocumentParser.cpp:126
> +    if (auto* document = this->document())
> +        document->beforeLoadImageEventSender().dispatchPendingEvents();

I’d like this to say one of these two things:

    ImageLoader::dispatchPendingBeforeLoadEvents(*document);

Or:

    document->dispatchPendingBeforeLoadEvents();

It’s better to have the details of how this is done be a secret of EventSender and Document rather than something XMLDocumentParser does by working with the event sender directly. This is how it was factored before and I would like to preserve that.

[Darin Adler]

Comment on attachment 237229 [details]
Patch

All of my comments from the previous patch apply to this one too. It’s OK to land it like this, but I have many stylistic and factoring concerns about this for the long term.

[Andreas Kling]

Created attachment 237260 [details]
Smaller patch

[Simon Fraser (smfr)]

Comment on attachment 237260 [details]
Smaller patch

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

> Source/WebCore/dom/Document.cpp:2380
>      if (f) {

Please rename f to frame.

[Andreas Kling]

(In reply to comment #9)
> (From update of attachment 237260 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237260&action=review
> 
> > Source/WebCore/dom/Document.cpp:2380
> >      if (f) {
> 
> Please rename f to frame.

I'm gonna keep this patch to a minimum. Will clean up names etc on trunk afterwards.

[WebKit Commit Bot]

Comment on attachment 237260 [details]
Smaller patch

Clearing flags on attachment: 237260

Committed r173028: <http://trac.webkit.org/changeset/173028>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.