Closed Bug 1274775 Opened 4 years ago Closed 4 years ago

Exception in WebExtensions background canvas

Categories

(WebExtensions :: Untriaged, defect, P3)

46 Branch
defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: glen.little, Assigned: kmag)

References

(Depends on 1 open bug)

Details

(Whiteboard: triaged)

Attachments

(1 file)

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.
Also happens in Firefox Developer Edition 48.0a2
Component: Untriaged → WebExtensions
Product: Firefox → Toolkit
Status: UNCONFIRMED → NEW
Ever confirmed: true
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)
Priority: -- → P3
Whiteboard: triaged
Blocks: 1197451
Flags: needinfo?(kmaglione+bmo)
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/
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)
(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.
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)
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.
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.
https://hg.mozilla.org/integration/fx-team/rev/f55699ce1dbdef37a792f3f8787b8b8895511a1e
Bug 1274775 - Make sure background pages are created with a PresShell. r=billm
https://hg.mozilla.org/mozilla-central/rev/f55699ce1dbd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee: nobody → kmaglione+bmo
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.