1274775 - Exception in WebExtensions background canvas

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[glen.little]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.102 Safari/537.36

Steps to reproduce:

In the startup code in the background script, I have something like this:

    var canvas = document.createElement('canvas');
    canvas.width = 19;
    canvas.height = 19;
    
    var context = canvas.getContext('2d');
    context.clearRect(0, 0, canvas.width, canvas.height);
    context.fillStyle = 'white';

    context.font = "9px sans-serif"; // fails
    context.fillText('Test', 0, 7);  // fails

When the `context.font` assignment is made, an exception is thrown:

    Exception { message: "", result: 2147500037, name: "NS_ERROR_FAILURE"....}

If the `context.font` assignment is removed, the next line will fail with the same error.

If the same code is later done in a popup window or a tab, it succeeds.


Actual results:

Get an exception (with a blank message).


Expected results:

Canvas image should be created.

Comment 1

[glen.little]

Also happens in Firefox Developer Edition 48.0a2

Comment 2

[Kris Maglione [:kmag]]

It looks like this is happening because documents in our windowless browsers are ending up without PresShells. I guess that makes a certain amount of sense, but a PresShell is required for working out font metrics, so I suppose we'll need to do something about it.

I'll try to look into it some more this week.

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1274775 - Make sure background pages are created with a PresShell.

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

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8769392 [details]
Bug 1274775 - Make sure background pages are created with a PresShell.

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

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Are the test changes needed even if we didn't have this presshell issue? I see you marked a bunch of test timeouts as deps.

Comment 6

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #5)
> Are the test changes needed even if we didn't have this presshell issue? I
> see you marked a bunch of test timeouts as deps.

Yes. Those tests all have races that are causing intermittents, but this patch changes the startup timing enough that they happen much more often.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Kris Maglione [:kmag] from comment #6)
> (In reply to Bill McCloskey (:billm) from comment #5)
> > Are the test changes needed even if we didn't have this presshell issue? I
> > see you marked a bunch of test timeouts as deps.
> 
> Yes. Those tests all have races that are causing intermittents, but this
> patch changes the startup timing enough that they happen much more often.

For that problem, what if we allow Management.emit event handlers to return a promise:
http://searchfox.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#1361
This means we won't be using the event emitter pattern anymore, but... patterns are stupid anyway.

We could collect promises for all the manifest directives and then do Promise.all on them, along with the broadcast("Extension:Startup") promise. That way, tests that |yield extension.startup()| would work automatically.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I guess the XUL document thing is okay. I'd much rather fix it at the platform level, but I don't want to hold up a fix for a bug. If comment 7 is okay with you, I'm happy to r+ a new patch with that addressed.

Comment 9

[Kris Maglione [:kmag]]

Yeah, I'd actually considered changing startup() to resolve after the background scripts were loaded, after I'd fixed the first five or so tests, so it seems like a good idea.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8769392 [details]
Bug 1274775 - Make sure background pages are created with a PresShell.

https://reviewboard.mozilla.org/r/63324/#review62006

OK, cancelling for now.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8769392 [details]
Bug 1274775 - Make sure background pages are created with a PresShell.

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

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8769392 [details]
Bug 1274775 - Make sure background pages are created with a PresShell.

https://reviewboard.mozilla.org/r/63324/#review63328

::: toolkit/components/extensions/ext-backgroundPage.js:55
(Diff revision 3)
>      let chromeShell = chromeWebNav.QueryInterface(Ci.nsIInterfaceRequestor)
> -                                  .getInterface(Ci.nsIDocShell);
> +                                  .getInterface(Ci.nsIDocShell)
> +                                  .QueryInterface(Ci.nsIWebNavigation);

This is a little confusing since you could just QI directly to nsIWebNav. It looks like you need both. Can you do them as separate statements so someone doesn't try to "optimize" this?

::: toolkit/components/extensions/ext-backgroundPage.js:109
(Diff revision 3)
> -          if (!consoleWindow) {
> -            let {require} = Cu.import("resource://devtools/shared/Loader.jsm", {});
> +        let {require} = Cu.import("resource://devtools/shared/Loader.jsm", {});
> -            require("devtools/client/framework/devtools-browser");
> +        require("devtools/client/framework/devtools-browser");
> +
> -            let hudservice = require("devtools/client/webconsole/hudservice");
> +        let hudservice = require("devtools/client/webconsole/hudservice");
> -            hudservice.toggleBrowserConsole().catch(Cu.reportError);
> +        hudservice.openBrowserConsoleOrFocus();

This is going to conflict with Rob's changes, but you should just be able to move it to the Extension.jsm observer.

Comment 13

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/63324/#review63328

> This is a little confusing since you could just QI directly to nsIWebNav. It looks like you need both. Can you do them as separate statements so someone doesn't try to "optimize" this?

I'll add a comment. It's a bit confusing exactly what's going on no matter how it's written.

Comment 14

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/f55699ce1dbdef37a792f3f8787b8b8895511a1e
Bug 1274775 - Make sure background pages are created with a PresShell. r=billm

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f55699ce1dbd