Bug 110960 – Web Inspector: [Protocol] Turn SecurityOrigin into an object containing "domainSetInDOM"

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Web Inspector (Deprecated)

[Alexander Pavlov (apavlov)]

A document.setDomain() invocation modifies the associated SecurityOrigin, so equals() no longer works against a SecurityOrigin that has been created from the same string but without the domain set.

[Alexander Pavlov (apavlov)]

Created attachment 190474 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 190474 [details]
Patch

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

> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:641
> +    if (!frame)

It is odd that frame nullity check is only present here, not in requestDatabaseNames and requestData.
Looks like findFrameWithSecurityOrigin call actually belongs to assertDocument method that should not set any value to errorString as discussed offline.

> Source/WebCore/inspector/InspectorPageAgent.cpp:981
> +    if (errorString)

I would remove this: it is pretty clear that the frame was not found from the return value and we have decided that we don't need this kind of messages on front-end anyway.
Let's keep "Invalid security origin" message only.

> Source/WebCore/inspector/InspectorPageAgent.h:164
> +    void securityOriginChanged(Frame*, SecurityOrigin*);

I don't see neither calls nor implementation of this method. Did you mean to put it into another patch?

> Source/WebCore/inspector/front-end/ResourceTreeModel.js:669
> +    var object = JSON.parse(id);

I think ResourceTreeModel should assign ids to security origin and keep track of them providing securityOriginForId and securityOriginForUrlAndDomainSetInDOM getters instead of using JSON.parse/stringify methods.

> Source/WebCore/inspector/front-end/ResourceTreeModel.js:677
> +    get id()

please use functions instead of getters and setters.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:1888
> +    if (origin.url === "file://")

You should extract this logic, put it into SecurityOrigin object and reuse for file systems and IndexedDB

[Alexander Pavlov (apavlov)]

Created attachment 190727 [details]
Patch

[Early Warning System Bot]

Comment on attachment 190727 [details]
Patch

Attachment 190727 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16782410

[Early Warning System Bot]

Comment on attachment 190727 [details]
Patch

Attachment 190727 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16768770

[Alexander Pavlov (apavlov)]

Created attachment 190731 [details]
Patch

[WebKit Review Bot]

Comment on attachment 190731 [details]
Patch

Attachment 190731 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16795251

New failing tests:
inspector/extensions/extensions-eval-content-script.html
inspector/extensions/extensions-audits-content-script.html

[WebKit Review Bot]

Comment on attachment 190731 [details]
Patch

Attachment 190731 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16792090

New failing tests:
inspector/extensions/extensions-eval-content-script.html
inspector/extensions/extensions-audits-content-script.html

[Alexander Pavlov (apavlov)]

Created attachment 190926 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 190926 [details]
Patch

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

> Source/WebCore/inspector/front-end/ResourceTreeModel.js:392
> +WebInspector.ResourceTreeModel.SecurityOriginTracker = function(model)

Let's extract SecurityOriginTracker related rafactoring from this patch.

> Source/WebCore/inspector/front-end/ResourceTreeModel.js:469
> +        return origin || null;

securityOrigin is never null at this point.

[Alexander Pavlov (apavlov)]

Created attachment 190932 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 190932 [details]
Patch

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

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:644
> +    ScriptExecutionContext* scriptExecutionContext = assertScriptExecutionContextForOrigin(error, securityOrigin.get());

Looks like all assertScriptExecutionContextForOrigin calls should be replaced with findFrameForOrigin

[Vsevolod Vlasov]

Comment on attachment 190932 [details]
Patch

As discussed let's extract SecurityOriginTracker in a separate patch.

[Andrey Kosyakov]

Comment on attachment 190932 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:123
> +                    { "name": "securityOrigin", "$ref": "SecurityOrigin", "description": "Frame document's security origin." },

domainSetInDOM which you add to SecurityOrigin may change at arbitrary moment, right? Should it really be a property we keep in ResourceTreeModel? Other frame properties do not update unless navigation happens, and we do not seem to introduce a notification that will let front-end keep domainSetInDOM up to date. So this field is basically going to have value that does not necessarily match the reality in back-end.

> Source/WebCore/inspector/front-end/ExtensionAuditCategory.js:34
> + * @param {WebInspector.SecurityOrigin} extensionOrigin

I don't think that's right.

> Source/WebCore/inspector/front-end/ExtensionServer.js:698
> +    /**
> +     * @param {Event} event
> +     */

This patch is already quite big, could you please extract the "add missing annotations" part as a separate patch?

> Source/WebCore/inspector/front-end/ResourceTreeModel.js:202
> +        return this._securityOriginsById[id] || null;

why is "|| null" necessary?

> Source/WebCore/inspector/front-end/ResourceTreeModel.js:218
> +        return Object.keys(this._securityOriginFrameCount).map(converter.bind(this));

why not Object.values(this._securityOriginsById)?

> Source/WebCore/inspector/front-end/RuntimeModel.js:417
> +     * @param {WebInspector.SecurityOrigin} securityOrigin

This function used to use securityOrigin to identify the isolated world within the frame, and string was good for that. Now you pass the object that isn't really good for identification, as one of its field may change. We still only use securityOrigin.url() for actual matching, which is good -- but why not just pass that string directly as a function argument. The same applies for other location where securityOrigin is used for identification, rather than for access control.

[Alexander Pavlov (apavlov)]

Comment on attachment 190932 [details]
Patch

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

>> Source/WebCore/inspector/Inspector.json:123
>> +                    { "name": "securityOrigin", "$ref": "SecurityOrigin", "description": "Frame document's security origin." },
> 
> domainSetInDOM which you add to SecurityOrigin may change at arbitrary moment, right? Should it really be a property we keep in ResourceTreeModel? Other frame properties do not update unless navigation happens, and we do not seem to introduce a notification that will let front-end keep domainSetInDOM up to date. So this field is basically going to have value that does not necessarily match the reality in back-end.

Please see https://bugs.webkit.org/show_bug.cgi?id=110855. I've got a patch pending all these SecurityOrigin changes.

>> Source/WebCore/inspector/front-end/ExtensionAuditCategory.js:34
>> + * @param {WebInspector.SecurityOrigin} extensionOrigin
> 
> I don't think that's right.

Sorry, it didn't get caught by the compiler :( Will revert in the master patch.

>> Source/WebCore/inspector/front-end/ExtensionServer.js:698
>> +     */
> 
> This patch is already quite big, could you please extract the "add missing annotations" part as a separate patch?

All this was reverted.

>> Source/WebCore/inspector/front-end/ResourceTreeModel.js:202
>> +        return this._securityOriginsById[id] || null;
> 
> why is "|| null" necessary?

In order to stay consistent with the JsDoc (otherwise we'd have to write {...|undefined}).

>> Source/WebCore/inspector/front-end/ResourceTreeModel.js:218
>> +        return Object.keys(this._securityOriginFrameCount).map(converter.bind(this));
> 
> why not Object.values(this._securityOriginsById)?

Good point.

>> Source/WebCore/inspector/front-end/RuntimeModel.js:417
>> +     * @param {WebInspector.SecurityOrigin} securityOrigin
> 
> This function used to use securityOrigin to identify the isolated world within the frame, and string was good for that. Now you pass the object that isn't really good for identification, as one of its field may change. We still only use securityOrigin.url() for actual matching, which is good -- but why not just pass that string directly as a function argument. The same applies for other location where securityOrigin is used for identification, rather than for access control.

You are right, this was reverted.

[Alexander Pavlov (apavlov)]

For the goals of storage-related tasks in Web Inspector, string-based comparison of SecurityOrigins works fine, since the domainSetInDOM field is disregarded for storage.