Bug 103793 – Associate URLs with GraphicsContext3D instances created for WebGL

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

In order to improve the quality of crash reports and lost context notifications, a URL needs to be associated with the GraphicsContext3D instances created on behalf of WebGL.

As of the fix for Bug 101826, the FrameLoaderClient gets the opportunity to veto WebGL context creation, but the context creation code itself does not have enough information to determine the URL of the page which is creating the context. This information is most easily passed down from WebCore to the platform.

[Kenneth Russell]

Created attachment 177069 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 177069 [details]
Patch

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

> Source/Platform/chromium/public/WebGraphicsContext3D.h:112
> +        WebString urlOfCreatingDocument;

Why a WebString an not a WebURL?

[Kenneth Russell]

(In reply to comment #3)
> (From update of attachment 177069 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177069&action=review
> 
> > Source/Platform/chromium/public/WebGraphicsContext3D.h:112
> > +        WebString urlOfCreatingDocument;
> 
> Why a WebString an not a WebURL?

No good reason. It turns out there was a technical problem using WebURL here, but it wasn't difficult to fix in the gyp files. New patch coming up.

[Kenneth Russell]

Created attachment 177411 [details]
Patch

[Adam Barth]

Comment on attachment 177411 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:428
> +    attributes.topDocumentURL = document->frame()->tree()->top()->document()->url();

document->topDocument() will save you a lot of typing.

[Kenneth Russell]

Created attachment 177643 [details]
Patch for landing

[Kenneth Russell]

Comment on attachment 177411 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:428
>> +    attributes.topDocumentURL = document->frame()->tree()->top()->document()->url();
> 
> document->topDocument() will save you a lot of typing.

Fixed, thanks.

[Kenneth Russell]

Committed r136609: <http://trac.webkit.org/changeset/136609>

[Stephen White]

Reverted r136609 for reason:

Breaking Chrome/Mac Debug builds.

Committed r136734: <http://trac.webkit.org/changeset/136734>

[Kenneth Russell]

Example broken build: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac%20Builder%20%28dbg%29/builds/34923

[Kenneth Russell]

@thakis indicates this bot is using the shared library build.

[Kenneth Russell]

I tried, but it is infeasible to pass an instance of WebURL (not a reference) across the WebKit boundary, in particular in the component build. The problem is that WebURL.h has inlined constructors which call out-of-line code in the googleurl library, and there is just no way using GYP to express that dependents of targets in WebKit.gyp need to depend on (i.e., include on their link line) the googleurl library. Neither export_dependent_settings nor direct_dependent_settings do this; note for example that the icuuc library (currently in the export_dependent_settings of WebKit.gyp:webkit) does not show up on the link line of targets that depend on webkit, such as src/webkit/gpu/webkit_gpu.gypi:webkit_gpu.

If I try to use a WebURL here then some (but not all) downstream targets of the WebKit API will have to mysteriously add a googleurl dependency in the component build. I found at least two targets (src/webkit/gpu/webkit_gpu.gypi:webkit_gpu and src/ui/compositor/compositor.gyp:compositor) which need it, but I'm not going to pursue this further as it would just add a time bomb to the code base.

Tomorrow I'll recast this patch to use WebString. abarth suggested adding a FIXME indicating why it doesn't use WebURL; this comment documents the reason.

[Kenneth Russell]

Created attachment 178083 [details]
Revised patch using WebString

[Kenneth Russell]

abarth doesn't seem to be around today, but yesterday indicated offline that the change to WebString is OK. I can't afford another delay of this patch and it looks good on the EWS so am landing it by hand now.

[Kenneth Russell]

Committed r136907: <http://trac.webkit.org/changeset/136907>

[Adam Barth]

Comment on attachment 178083 [details]
Revised patch using WebString

Delayed LGTM