Bug 212461 – AutoTrader crashed while browsing search results

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

AutoTrader crashed while browsing search results

[Dean Jackson]

We've noticed lots of crashes on iOS since swapping to the ANGLE backend. In every case, the stack trace points to the main thread performing some GL operations instead of the web thread. This indicates it is specific to WebKit1, and the fact that CoreAnimation is the one that calls WebGLLayer's display method on a separate thread.

[Dean Jackson]

rdar://60733185

[Dean Jackson]

The fix for this looks like it might address 212251 as well.

[Dean Jackson]

Created attachment 400450 [details]
Patch

[Dean Jackson]

I was seeing some strange failures in WebGL tests locally (test timeouts), but only on WK2. Looking forward to the EWS results.

I wonder if there could be an issue with the timing of the prepare v display calls. If possible, I'd love to know there is a guarantee that display will always be called after prepare, and we'd never get two displays or prepares in a row.

[Sam Weinig]

Comment on attachment 400450 [details]
Patch

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

> Source/WebCore/html/CanvasBase.cpp:55
> +HashSet<CanvasBase*>& CanvasBase::instancesRequiringPreparationForDisplay()
> +{
> +    static NeverDestroyed<HashSet<CanvasBase*>> instances;
> +    return instances;
> +}

I think doing this in CanvasBase is probably a mistake, given it is not safe to use this from multiple threads (and I think OffscreenCanvas works on a non-main thread). I realize this is not a problem currently, as needsPreparationForDisplay() is only true for HTMLCanvasElement, but I think to make this a bit more robust, I would move this set up to HTMLCanvasElement as well. One way to do this would be to replace needsPreparationForDisplay() with a virtual function like addToRequiringPreparationForDisplaySetIfNeeded() (but with a better name) that only HTMLCanvasElement would implement in a meaningful way. 

I am also generally dubious of global state like this, and if possible aim to avoid it. Can the set of HTMLCanvasElements live on the Page or something like that instead?

[Dean Jackson]

Comment on attachment 400450 [details]
Patch

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

>> Source/WebCore/html/CanvasBase.cpp:55
>> +}
> 
> I think doing this in CanvasBase is probably a mistake, given it is not safe to use this from multiple threads (and I think OffscreenCanvas works on a non-main thread). I realize this is not a problem currently, as needsPreparationForDisplay() is only true for HTMLCanvasElement, but I think to make this a bit more robust, I would move this set up to HTMLCanvasElement as well. One way to do this would be to replace needsPreparationForDisplay() with a virtual function like addToRequiringPreparationForDisplaySetIfNeeded() (but with a better name) that only HTMLCanvasElement would implement in a meaningful way. 
> 
> I am also generally dubious of global state like this, and if possible aim to avoid it. Can the set of HTMLCanvasElements live on the Page or something like that instead?

I originally started with Page calling forEachDocument which would then find the HTMLCanvasElements that the Document considered dirty. Maybe that's a better solution - keep the set there. I ended up with the current approach when I realised I could take a shortcut.

I'll fix it to have Page call into each Document, and the set of canvii that need preparation will live there. An HTMLCanvasElement always has a Document.

[Kenneth Russell]

Does this address both Bug 212271 and Bug 210153? May I block those two bugs on this one since it sounds like the first fix will be done here?

[Kenneth Russell]

Comment on attachment 400450 [details]
Patch

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

> Source/WebCore/html/CanvasBase.h:106
> +    static HashSet<CanvasBase*>& instancesRequiringPreparationForDisplay();

In the next iteration of this patch, could you add some documentation indicating that the set of canvases is transient, and cleared out at the end of each rendered frame? That would help address concerns about the lifetime of the entries in the collection.

Also wondering whether this should hold RefPtrs to the target canvases for GC safety.

> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/WebGLPrepareDisplayOnWebThread.mm:102
> +    [jsContext evaluateScript:@"loadColorIntoTexture(128, 128, 255, 255)"];

Is this color verified by the test harness? It would be ideal if it were.

[Dean Jackson]

Comment on attachment 400450 [details]
Patch

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

>> Source/WebCore/html/CanvasBase.h:106
>> +    static HashSet<CanvasBase*>& instancesRequiringPreparationForDisplay();
> 
> In the next iteration of this patch, could you add some documentation indicating that the set of canvases is transient, and cleared out at the end of each rendered frame? That would help address concerns about the lifetime of the entries in the collection.
> 
> Also wondering whether this should hold RefPtrs to the target canvases for GC safety.

This is a good point. I think it is a mistake in the current patch that the set is cleared by Page. Instead Page should not need to know it must clear the set, and instead call into something that does the preparation and clears itself.

Also agree on RefPtr.

[Dean Jackson]

Comment on attachment 400450 [details]
Patch

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

>> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/WebGLPrepareDisplayOnWebThread.mm:102
>> +    [jsContext evaluateScript:@"loadColorIntoTexture(128, 128, 255, 255)"];
> 
> Is this color verified by the test harness? It would be ideal if it were.

It isn't. The goal here was to be doing something from outside the UIWebView that would cause a conflict. However, I'm pretty sure this doesn't do that any more than simply calling draw would.

I do want to get to a point where I can have an API test that manipulates the WebGL and gets back the contents of the canvas.

[Dean Jackson]

Created attachment 400638 [details]
Patch

[Dean Jackson]

*** Bug 210153 has been marked as a duplicate of this bug. ***

[Sam Weinig]

Comment on attachment 400638 [details]
Patch

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

> Source/WebCore/ChangeLog:41
> +            need to run code before being composited. Collect
> +            these objects in a static HashSet, so that the Page

You don't have a static set anymore, but a per-document one.

> Source/WebCore/html/HTMLCanvasElement.cpp:1010
> +        document().canvasNeedsPreparationForDisplay(this);

Seems like the HTMLCanvasElement needs to also remove itself from the Document::canvasNeedsPreparationForDisplay set if its destroyed. It may also want to reset any of this state if it moves between documents.

> Source/WebCore/html/HTMLCanvasElement.h:187
> +    bool m_hasInformedNeedForDisplayPreparation { false };

This variable name is a bit awkward.

[Dean Jackson]

Comment on attachment 400638 [details]
Patch

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

>> Source/WebCore/ChangeLog:41
>> +            these objects in a static HashSet, so that the Page
> 
> You don't have a static set anymore, but a per-document one.

:thumbsup:

>> Source/WebCore/html/HTMLCanvasElement.cpp:1010
>> +        document().canvasNeedsPreparationForDisplay(this);
> 
> Seems like the HTMLCanvasElement needs to also remove itself from the Document::canvasNeedsPreparationForDisplay set if its destroyed. It may also want to reset any of this state if it moves between documents.

Will do.

>> Source/WebCore/html/HTMLCanvasElement.h:187
>> +    bool m_hasInformedNeedForDisplayPreparation { false };
> 
> This variable name is a bit awkward.

I'm getting rid of it as I move to making Document a CanvasObserver. I worried about the (slight) cost of the hash lookup each time the canvas draws something (although the document will ask the canvas if it needs preparation before doing that).

[Kenneth Russell]

Comment on attachment 400638 [details]
Patch

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

Looks good overall; a couple of questions.

> Source/WebCore/ChangeLog:27
> +        which had been difficult to diagnose.

This block should be removed now that it's been determined that https://bugs.webkit.org/show_bug.cgi?id=212277 was the root cause.

> Source/WebCore/dom/Document.h:1819
> +    HashSet<HTMLCanvasElement*> m_canvasesNeedingDisplayPreparation;

Is RefPtr<HTMLCanvasElement> not an option because it will introduce a graph cycle? That might address the lifetime consideration.

>> Source/WebCore/html/HTMLCanvasElement.cpp:1010
>> +        document().canvasNeedsPreparationForDisplay(this);
> 
> Seems like the HTMLCanvasElement needs to also remove itself from the Document::canvasNeedsPreparationForDisplay set if its destroyed. It may also want to reset any of this state if it moves between documents.

Is document() typically non-null in HTMLCanvasElement::~HTMLCanvasElement? If not, when would be a good time to unregister it?

[Dean Jackson]

Created attachment 400646 [details]
Patch

[Dean Jackson]

Comment on attachment 400638 [details]
Patch

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

>> Source/WebCore/ChangeLog:27
>> +        which had been difficult to diagnose.
> 
> This block should be removed now that it's been determined that https://bugs.webkit.org/show_bug.cgi?id=212277 was the root cause.

Removed.

>> Source/WebCore/dom/Document.h:1819
>> +    HashSet<HTMLCanvasElement*> m_canvasesNeedingDisplayPreparation;
> 
> Is RefPtr<HTMLCanvasElement> not an option because it will introduce a graph cycle? That might address the lifetime consideration.

It probably would be fine to use a RefPtr but other sets/lists on document don't. As long as the lifetime is guaranteed via CanvasObserver I think we're ok.

>>>> Source/WebCore/html/HTMLCanvasElement.cpp:1010
>>>> +        document().canvasNeedsPreparationForDisplay(this);
>>> 
>>> Seems like the HTMLCanvasElement needs to also remove itself from the Document::canvasNeedsPreparationForDisplay set if its destroyed. It may also want to reset any of this state if it moves between documents.
>> 
>> Will do.
> 
> Is document() typically non-null in HTMLCanvasElement::~HTMLCanvasElement? If not, when would be a good time to unregister it?

document() is always a Ref<>, never null.

[Dean Jackson]

Committed r262366: <https://trac.webkit.org/changeset/262366>

[Ryan Haddad]

(In reply to Dean Jackson from comment #19)
> Committed r262366: <https://trac.webkit.org/changeset/262366>
As win-ews detected, this change broke the Windows build:
C:\cygwin\worker\win10-release\build\Source\WebCore\html/HTMLCanvasElement.cpp(1028,61): error C2027: use of undefined type 'WebCore::WebGLRenderingContextBase' (compiling source file C:\cygwin\worker\win10-release\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-950a39b6-5.cpp) [C:\cygwin\worker\win10-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

https://build.webkit.org/builders/Apple%20Win%2010%20Release%20%28Build%29/builds/13995/steps/compile-webkit/logs/stdio

[Ryan Haddad]

(In reply to Ryan Haddad from comment #20)
> (In reply to Dean Jackson from comment #19)
> > Committed r262366: <https://trac.webkit.org/changeset/262366>
> As win-ews detected, this change broke the Windows build:
> C:\cygwin\worker\win10-release\build\Source\WebCore\html/HTMLCanvasElement.
> cpp(1028,61): error C2027: use of undefined type
> 'WebCore::WebGLRenderingContextBase' (compiling source file
> C:\cygwin\worker\win10-
> release\build\WebKitBuild\Release\DerivedSources\WebCore\unified-
> sources\UnifiedSource-950a39b6-5.cpp)
> [C:\cygwin\worker\win10-
> release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
> 
> https://build.webkit.org/builders/Apple%20Win%2010%20Release%20%28Build%29/
> builds/13995/steps/compile-webkit/logs/stdio
This was fixed with https://trac.webkit.org/changeset/262418/webkit

[Simon Fraser (smfr)]

Comment on attachment 400646 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:8590
> +        auto refCountedCanvas = makeRefPtr(canvas);

Normally we call this protectedFoo,

> Source/WebCore/dom/Document.cpp:8601
> +        auto* canvas = downcast<HTMLCanvasElement>(&canvasBase);
> +        if (canvas->needsPreparationForDisplay())
> +            m_canvasesNeedingDisplayPreparation.add(canvas);

Would be nicer if canvas were a reference.

> Source/WebCore/dom/Document.h:1823
> +    HashSet<HTMLCanvasElement*> m_canvasesNeedingDisplayPreparation;

Should this have been a weak hash set?

> Source/WebCore/platform/graphics/cocoa/WebGLLayer.h:65
> +    BOOL _prepared;

preparedForWhat?

> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/WebGLPrepareDisplayOnWebThread.mm:39
> +@interface WebGLPrepareDisplayOnWebThreadDelegate : NSObject <UIWebViewDelegate> {
> +}

Are the {} necessary?

[Dean Jackson]

Addressed comments in https://bugs.webkit.org/show_bug.cgi?id=212647