Exception in WebExtensions background canvas

RESOLVED FIXED in Firefox 50

Status

WebExtensions
Untriaged
P3
normal
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: glen.little, Assigned: kmag)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

46 Branch
mozilla50
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Also happens in Firefox Developer Edition 48.0a2

Updated

2 years ago
Component: Untriaged → WebExtensions
Product: Firefox → Toolkit
(Assignee)

Updated

2 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

2 years ago
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.
Flags: needinfo?(kmaglione+bmo)

Updated

2 years ago
Priority: -- → P3
Whiteboard: triaged
(Assignee)

Updated

2 years ago
Blocks: 1197451
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 3

2 years ago
Created attachment 8769392 [details]
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/
Attachment #8769392 - Flags: review?(wmccloskey)
(Assignee)

Comment 4

2 years ago
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/
(Assignee)

Updated

2 years ago
(Assignee)

Updated

2 years ago
Blocks: 1272869
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.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 6

2 years ago
(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.
Flags: needinfo?(kmaglione+bmo)
(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.
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.
(Assignee)

Comment 9

2 years ago
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 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.
Attachment #8769392 - Flags: review?(wmccloskey)
(Assignee)

Comment 11

2 years ago
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/
Attachment #8769392 - Flags: review?(wmccloskey)
Attachment #8769392 - Flags: review?(wmccloskey) → review+
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.
(Assignee)

Comment 13

2 years ago
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.
(Assignee)

Comment 14

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

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f55699ce1dbd
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Updated

2 years ago
Blocks: 1285557
(Assignee)

Updated

2 years ago
Assignee: nobody → kmaglione+bmo

Updated

a month ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.