1293132 - The "context" type in Schemas.inject and Schemas.load are unclear

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Rob Wu [:robwu]]

For a while I've been thinking that the "Context" in Schemas.jsm is related to ExtensionContext/BaseContext, because of the similar properties and the fact that Schemas.normalize is sometimes invoked with an ExtensionContext (see e.g. connectNative / sendNativeMessage).

Further, there are methods names that suggest that there is no return value (callFunctionNoReturn), yet the body contains a return statement that takes the return value from an arbitrary method.

These issues makes it more difficult to reason about the logic of schema-generated APIs, so I'm going to submit a patch that documents how they work as I understand it.

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1293132 - Document and enforce contract for Schemas.inject and Schemas.normalize

Review commit: https://reviewboard.mozilla.org/r/69882/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69882/

Comment 2

[Rob Wu [:robwu]]

Comment on attachment 8778725 [details]
Bug 1293132 - Document and enforce contract for Schemas.inject and Schemas.normalize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69882/diff/1-2/

Comment 3

[Rob Wu [:robwu]]

Depends on bug 1295473 to not fail unit tests (runtime.sendMessage is currently implemented using callFunctionNoReturn, while it should be handled by callAsyncFunction).

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8778725 [details]
Bug 1293132 - Document and enforce contract for Schemas.inject and Schemas.normalize

https://reviewboard.mozilla.org/r/69882/#review69646

::: toolkit/components/extensions/Schemas.jsm:165
(Diff revision 2)
> +  /**
> +   * @param {nsIURI} url
> +   * @returns {boolean} Whether the context may load |url|.
> +   */

Please provide a description for the method and its parameters. Also, please use backticks rather than |...| around symbol names. Same for other methods.

::: toolkit/components/extensions/Schemas.jsm:166
(Diff revision 2)
>    get principal() {
>      return this.params.principal || Services.scriptSecurityManager.createNullPrincipal({});
>    }
>  
> +  /**
> +   * @param {nsIURI} url

The `url` parameter is a string, not a nsIURI.

::: toolkit/components/extensions/Schemas.jsm:248
(Diff revision 2)
> -   * Logs the given error to the console. May be overridden to enable
> +   * Logs the given error to the console.
> -   * custom logging.

I don't see a reason for this change.

Comment 5

[Rob Wu [:robwu]]

Comment on attachment 8778725 [details]
Bug 1293132 - Document and enforce contract for Schemas.inject and Schemas.normalize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69882/diff/2-3/

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8778725 [details]
Bug 1293132 - Document and enforce contract for Schemas.inject and Schemas.normalize

https://reviewboard.mozilla.org/r/69882/#review69664

Comment 7

[Rob Wu [:robwu]]

Actually no checkin-needed yet. This patch is blocked on bug 1295473 to not fail tests as I explained in comment 3.

Comment 8

[Rob Wu [:robwu]]

Comment on attachment 8778725 [details]
Bug 1293132 - Document and enforce contract for Schemas.inject and Schemas.normalize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69882/diff/3-4/

Comment 9

[Rob Wu [:robwu]]

The other patch was pushed to autoland. Please land this one after the patch for 1295473 lands.

Comment 10

[Rob Wu [:robwu]]

Comment on attachment 8778725 [details]
Bug 1293132 - Document and enforce contract for Schemas.inject and Schemas.normalize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69882/diff/4-5/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d71c400313a2
Document contract for Schemas.inject and Schemas.normalize r=kmag

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing test_ext_schemas_api_injection.js on Android's Sets test:

https://hg.mozilla.org/integration/autoland/rev/1c6f07b604f671e7a1844e6351e4b7174b58b2ef

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=2150586&repo=autoland

Comment 13

[Rob Wu [:robwu]]

Failure is caused by a different bug, https://bugzilla.mozilla.org/show_bug.cgi?id=1295082#c29, no action needed here.

Comment 14

[Rob Wu [:robwu]]

Comment on attachment 8778725 [details]
Bug 1293132 - Document and enforce contract for Schemas.inject and Schemas.normalize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69882/diff/5-6/

Comment 15

[Rob Wu [:robwu]]

test_ext_schemas_api_injection.js failed because its schema declared a function without return value, while the actual API did return a value. I fixed the test by adding the correct return type and confirmed locally that it runs as expected.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Try push has test_ext_storage_tab.html failures still.

Comment 17

[Rob Wu [:robwu]]

Test is looking good to me. The listed failures are caused by the absence of the patch for bug 1295473 on the try server. I ran the tests locally with that patch applied and the tests passed (and without the patch I see the same errors as on try).

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38ad193a808c
Document and enforce contract for Schemas.inject and Schemas.normalize r=kmag

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/38ad193a808c