1214174 - OOM exception on accessing some of the window properties in a background page

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: background-window-properties-OOM-exception-testcase.patch

I was experimenting with an webextension addon built using browserify, when the addon background page is loaded it raises an OOM exception.

Using gdb I managed to identify what is the exact piece of code which generates the exception:

this addon uses a number of npm modules which include the "object-keys" npm module as a dependency, when the "object-keys" runs in a background page then it raises an OOM exception during the following check (based on code from es5-shim):

https://github.com/ljharb/object-keys/blob/master/index.js#L36

it seems that accessing following window properties in the background page is what raises the following exceptions:

- window.sidebar: OOM exception
- window.external: OOM exception
- window.screenX: Illegal value exception
- window.screenY: Illegal value exception
- window.outerWidth: Illegal value exception
- window.outerHeight: Illegal value exception

This is probably related to the background page running inside a WindowlessBrowser, created using Services.appShell.createWindowlessBrowser.

I'm attaching a small test case which reproduces the issue.

Comment 1

[Andy McKay]

Is the OOM exception similar to this bug 1203394?

Comment 2

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

Yes. Both bugs are about doing things that are currently unsupported in windowless docshells.

Comment 4

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

I think the only thing we want to do here is see if our error messages are the same as Chrome's.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

These are the results of accessing the above properties from a background page in Chrome:

- window.sidebar: null (in Firefox should be probable a synonym of window.external) 
- window.external: object which implements External interface (https://dxr.mozilla.org/mozilla-central/source/dom/webidl/External.webidl)
- window.screenX, window.screenY, window.outerWidth, window.outerHeight should return 0

I've tracked down the above exceptions and they seem to be related to the following two issues:

- window.sidebar (and window.external) is implemented by a registered javascript component (https://dxr.mozilla.org/mozilla-central/source/dom/webidl/External.webidl) and it fails on initialization because it is not able to get the message manager from the background page's docShell (https://dxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSidebar.js#19)

- window.screenX, window.screenY, window.outerWidth, window.outerHeight raise exceptions because the WebBrowserChrome2Stub used in the windowless browser does not implement the nsIEmbeddingSiteWindow interface (in particular the GetDimensions method) and the DocShellTreeOwner is not able to calculate the position and size (https://dxr.mozilla.org/mozilla-central/source/embedding/browser/nsDocShellTreeOwner.cpp#554 and https://dxr.mozilla.org/mozilla-central/source/embedding/browser/nsDocShellTreeOwner.cpp#576)

I'm going to attach an updated test case and two more patches as proposed fixes of the about two issues.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1214174-background-window-properties-OOM-excepti.patch

This patch updates the test case to check the returned values of window.screenX, window.screenY, window.outerWidth and window.outerHeight.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0002-Bug-1214174-prevents-out-of-memory-exception-on-acce.patch

This patch prevents the sidebar registered component to raise an exception when accessed from a background page (because it is unable to get a message manager for the background window's docShell)

The sidebar's "addSearchProvider" method will fail when called from a background page, we should probably change the method to check "this.mm" and raise a proper error or ignore the request if the message manager is not defined.

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0003-Bug-1214174-windowless-WebBrowserChrome2Stub-should-.patch

This patch adds a shim implementation of "nsIEmbeddingSiteWindow" interface in the "WebBrowserChrome2Stub" class, which is associated to the windowless browsers created from the "nsAppShellService".

All the methods return NS_ERROR_NOT_IMPLEMENTED, besides the "GetDimensions" method which return the fake position and size values (all zeroes actually).

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

This is a try run of the above patches (actually of the same patches before the last rebase and minor tweaks on the testcase that I've applied today):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79b547cf4884

Comment 10

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

Comment on attachment 8675734 [details] [diff] [review]
0002-Bug-1214174-prevents-out-of-memory-exception-on-acce.patch

Review of attachment 8675734 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/search/nsSidebar.js
@@ +21,1 @@
>                      .getInterface(Ci.nsIDocShell)

Please indent these lines over too.

Comment 11

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

Comment on attachment 8675736 [details] [diff] [review]
0003-Bug-1214174-windowless-WebBrowserChrome2Stub-should-.patch

Review of attachment 8675736 [details] [diff] [review]:
-----------------------------------------------------------------

Please add a test for this stuff. Thanks!

::: xpfe/appshell/nsAppShellService.cpp
@@ +331,5 @@
>  }
>  
> +// nsIEmbeddingSiteWindow impl
> +NS_IMETHODIMP
> +WebBrowserChrome2Stub::GetDimensions(uint32_t flags, int32_t *x, int32_t *y, int32_t *cx, int32_t *cy)

In all this code please make sure that the * goes with the type. So |int32_t* x|.

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1214174-background-window-properties-OOM-excepti.patch

I rebased the patch and updated the commit message.

Even if I'm going to add a new test case which is going to test mostly the same issues in the "xpfe/appshell/test" dir, I'm not removing the original test case because of the different goals of the two:

- this one tests that the WebExtension background page behaves correctly with the es5-shim  
- the latter tests the fixes are correctly applied to any windowless browser (chrome privileged or content privileged)

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0002-Bug-1214174-prevents-out-of-memory-exception-on-acce.patch

Patch rebased, fixed indentation and tweaked as follows:

now it ignores any calls to nsSidebar.AddSearchProvider method (but logs an error in the browser console, with the error call stack)

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0003-Bug-1214174-windowless-WebBrowserChrome2Stub-should-.patch

Patch rebased and fixed coding style on the added WebBrowserChrome2Stub's methods

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0004-Bug-1214174-add-windowless-browser-test-case-r-billm.patch

This new patch adds the new test case. 

I added it in the "xpfs/appshell/test" but I noticed the only tests on the windowless browser are currently in the "docshell/test/chrome", 
if you prefer I can move the test case there.

The test case is not very different from the one in the first patch of this series, but it tests the fixes on both the content privileged and chrome privileged cases and it is located in a test suite which is more strictly related to the fixes position in the source tree.

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

Pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf2889d64570

Comment 17

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

Comment on attachment 8677703 [details] [diff] [review]
0001-Bug-1214174-background-window-properties-OOM-excepti.patch

Review of attachment 8677703 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/extensions/test/mochitest/test_ext_background_oom_bug.html
@@ +11,5 @@
> +<body>
> +
> +<script type="application/javascript;version=1.8">
> +
> +add_task(function* test_messenger() {

Please call this something else.

@@ +15,5 @@
> +add_task(function* test_messenger() {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    background: "(" + function() {
> +      // based on es5-shim / object.keys code
> +      var has = Object.prototype.hasOwnProperty;

This appears to be unused.

@@ +16,5 @@
> +  let extension = ExtensionTestUtils.loadExtension({
> +    background: "(" + function() {
> +      // based on es5-shim / object.keys code
> +      var has = Object.prototype.hasOwnProperty;
> +      var blacklistedKeys = {

Do you know what this blacklist is for?

@@ +32,5 @@
> +        outerWidth: 0,
> +        outerHeight: 0
> +      };
> +
> +      var hasAutomationEqualityBug = (function () {

I'm a little worried that this code is copied verbatim from a different project. I know it's open source, but I still worry that there could be licensing issues. Can you change the code so that it's more specific to our purpose? Maybe get rid of blacklistedKeys if possible as well as the |typeof window| check, since we don't need that. Also there's no reason to call it hasAutomationEqualityBug.

Comment 18

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

Comment on attachment 8677706 [details] [diff] [review]
0004-Bug-1214174-add-windowless-browser-test-case-r-billm.patch

Review of attachment 8677706 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent, thanks!

Comment 19

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-Bug-1214174-background-window-properties-OOM-excepti.patch

Patch updated with the suggested tweaks:

- testcase file renamed to "test_ext_background_window_properties.html"
- the testcase function renamed to "testBackgroundWindowProperties"
- removed the unused/unuseful part of the testcase code (basically all the parts of the original sources which are not useful to the test case)

Comment 20

[Luca Greco [:rpl] [:luca] [:lgreco]]

These patches are ready and I rebased it on a recent mozilla-central without any conflicts and there isn't any changes to apply, so I'm setting checkin-needed to it as it is.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dded486c96e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc1cb7ec5fdd
https://hg.mozilla.org/integration/mozilla-inbound/rev/87ed082565d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/829547cb8c6e

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dded486c96e4
https://hg.mozilla.org/mozilla-central/rev/dc1cb7ec5fdd
https://hg.mozilla.org/mozilla-central/rev/87ed082565d2
https://hg.mozilla.org/mozilla-central/rev/829547cb8c6e

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/dded486c96e4
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/dc1cb7ec5fdd
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/87ed082565d2
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/829547cb8c6e

Comment 24

[Florian Quèze [:florian]]

(In reply to Bill McCloskey (:billm) from comment #18)
> Comment on attachment 8677706 [details] [diff] [review]
> 0004-Bug-1214174-add-windowless-browser-test-case-r-billm.patch
> 
> Review of attachment 8677706 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Excellent, thanks!

Is this test something that should run both with and without e10s? It's not clear to me if browsers created by Services.appShell.createWindowlessBrowser without chrome privileges run in the child process or in the parent one.

Comment 25

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #24)

> Is this test something that should run both with and without e10s? It's not
> clear to me if browsers created by Services.appShell.createWindowlessBrowser
> without chrome privileges run in the child process or in the parent one.

We discussed this over IRC in #e10s: http://logs.glob.uno/?c=mozilla%23e10s&s=5+Apr+2016&e=5+Apr+2016#c64143

Relevant quote:
20:37:25 - gabor: mconley: windowlessBrowser is used in the parent process for webextensions, for the content process we just use sandboxes
20:37:50 - mconley: gabor: I see - there’s no intent for the browser to work remotely?
20:40:04 - gabor: mconley: I don't know what's billm's plan for out of process add-ons... so maybe... but I'm not aware of any cases where we want that
20:40:35 - mconley: gabor: okay. I figure billm wouldn’t have r+’d the patch if it didn’t fit long-range plans - especially without mentioning it in the bug