Complete test coverage for browser.windows.create

RESOLVED FIXED in Firefox 48

Status

RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

unspecified
mozilla48
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
browser.windows.create is missing coverage for:

* passing an invalid |tabId|
* passing an array of URLs to open in |url|
* passing in a value for |left|, |top|, |width| and |height|

Missing coverage can be seen at https://people.mozilla.org/~kmaglione/webextension-test-coverage/browser/components/extensions/ext-windows.js.html
(Assignee)

Updated

3 years ago
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.2 - Apr 4
Flags: blocking-webextensions+
I didn't get a chance to update the coverage tests last week, so some of this is already fixed.
(Assignee)

Comment 2

3 years ago
Oh. I've already started working on it. Do you think you'll be able to find time to generate a new coverage report so I can know what needs to be done?
I updated it just before I posted that comment.
(Assignee)

Comment 4

3 years ago
Thanks Kris. Before I saw your comment I added explicit tests for |left|, |top|, |width| and |height|. I see from the report that we are not officially lacking coverage in that area, but it seems like we still don't have actual tests for them. I.e., create a window with one of those options and verify that the window has the requested option. Do you think it's still worth adding tests for that? I found while doing it that I did get expected values for |left|, |width| and |height|, but for |top| I did not, and I'm not sure if that's a bug or not. It seemed that when I created a window and specified a value for |top|, after the window was created, if I asked for its |top| value the number was always off by around 12. Is that a concern, or is it perhaps expected?
Flags: needinfo?(kmaglione+bmo)
(In reply to Bob Silverberg [:bsilverberg] from comment #4)
> I see from the report that we are not officially lacking coverage in that
> area, but it seems like we still don't have actual tests for them. I.e.,
> create a window with one of those options and verify that the window has the
> requested option.

It's tested pretty thoroughly here:
https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_windows_size.js

> I found while doing it that I did get expected values for |left|, |width|
> and |height|, but for |top| I did not, and I'm not sure if that's a bug or
> not. It seemed that when I created a window and specified a value for |top|,
> after the window was created, if I asked for its |top| value the number was
> always off by around 12. Is that a concern, or is it perhaps expected?

Different platforms impose different restrictions on window size and position.
On OS-X, the window has to be below the top menu bar, and can't be larger than
the available area of the screen, so certain values won't be fully respected.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 6

3 years ago
Thanks Kris. I had looked at `browser_ext_windows_create.js` but not `browser_ext_windows_size.js`. This bug now only needs coverage added for:

* passing an invalid |tabId|
* passing an array of URLs to open in |url|
(Assignee)

Comment 7

3 years ago
Created attachment 8737894 [details]
MozReview Request: Bug 1261185 - Complete test coverage for browser.windows.create, r?kmag

Add coverage for:
* passing an invalid |tabId|
* passing an array of URLs to open in |url|

Review commit: https://reviewboard.mozilla.org/r/44165/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44165/
Attachment #8737894 - Flags: review?(kmaglione+bmo)
Comment on attachment 8737894 [details]
MozReview Request: Bug 1261185 - Complete test coverage for browser.windows.create, r?kmag

https://reviewboard.mozilla.org/r/44165/#review40785

::: browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js:19
(Diff revision 1)
>      };
>  
> +    let promiseTabUpdated = () => {
> +      return new Promise(resolve => {
> +        browser.tabs.onUpdated.addListener(function listener() {
> +          browser.tabs.onUpdated.removeListener(listener);

We should check for `updated.url`, and I think we probably need to wait for two onUpdated events.

::: browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js:121
(Diff revision 1)
> +        Promise.resolve(window)
> +      ]);
> +    }).then(([, window]) => {
> +      return browser.tabs.query({windowId: window.id});
> +    }).then(tabs => {
> +      browser.test.assertEq(2, tabs.length, "2 tabs were opened in new window");

We should also check that the URLs are correct.
Attachment #8737894 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 9

3 years ago
https://reviewboard.mozilla.org/r/44165/#review40785

> We should also check that the URLs are correct.

I mentioned why I am not doing that in the commit comment. I will see if I can use `updated.url` to wait for the correct URLs. Luca also mentioned that I might be able to use `webNavigation.onCommitted` but wasn't sure if we should mix API calls in a test.
(Assignee)

Comment 10

3 years ago
https://reviewboard.mozilla.org/r/44165/#review40785

> I mentioned why I am not doing that in the commit comment. I will see if I can use `updated.url` to wait for the correct URLs. Luca also mentioned that I might be able to use `webNavigation.onCommitted` but wasn't sure if we should mix API calls in a test.

Update: Luca suggests that `webNavigation.onCompleted` would be better.
https://reviewboard.mozilla.org/r/44165/#review40785

> Update: Luca suggests that `webNavigation.onCompleted` would be better.

I think `onUpdated` is best. We want to know when the internal state of the tab objects has been updated, which doesn't really have a well-defined correlation with navigation events.
(Assignee)

Comment 12

3 years ago
Comment on attachment 8737894 [details]
MozReview Request: Bug 1261185 - Complete test coverage for browser.windows.create, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44165/diff/1-2/
Attachment #8737894 - Flags: review?(kmaglione+bmo)
Comment on attachment 8737894 [details]
MozReview Request: Bug 1261185 - Complete test coverage for browser.windows.create, r?kmag

https://reviewboard.mozilla.org/r/44165/#review40843
Attachment #8737894 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 15

3 years ago
Comment on attachment 8737894 [details]
MozReview Request: Bug 1261185 - Complete test coverage for browser.windows.create, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44165/diff/2-3/
(Assignee)

Updated

3 years ago
Iteration: 48.2 - Apr 4 → 48.3 - Apr 18
Keywords: checkin-needed
backed out for memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=8443526&repo=fx-team
Flags: needinfo?(bob.silverberg)
(Assignee)

Comment 20

3 years ago
I cannot reproduce this on my machine, either when running that test in isolation or when running all the tests in `browser/components/extensions/test/browser/`. I notice from the failure [1], that this only seems to happen on OS X 10.10 and I'm on 10.11, so maybe that's why I cannot reproduce.

Kris, do you have any suggestions? Does anything in the new code look like an obvious culprit for these window leaks? 
Everyone: Does anyone else on the team have a box with OS X 10.10 on it?

[1] https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=27df9bc5af2ca94e203e132013a5958bd60e85fc&selectedJob=8437274
Flags: needinfo?(bob.silverberg) → needinfo?(kmaglione+bmo)
(Assignee)

Comment 21

3 years ago
I do see some leaks reported from a tab process (as opposed to the default process), which can be seen in my sample output at [1], but they don't trigger any failures, they appear to be the same both with and without my patch, and I don't see those window leaks. I just realized that I have a Mac Mini with 10.10 on it, so I'm going to try to repro on that.

[1] https://gist.github.com/bobsilverberg/b7a1f38b6cdde8b535306ac25bd701b2
(Assignee)

Comment 22

3 years ago
I tested this on my 10.10 machine and get the same results. I do not see these window leaks when running either a single test nor the full suite, and I tried both e10s and non-e10s. Do we know if this was a consistent issue after landing this change, or was it intermittent?
If you can't reproduce this, I'd say push to try and see if you can reproduce again there.

If you do manage to reproduce, the easiest thing to do is disable code until you can't. If that doesn't help, someone's going to have to look through cycle collector graphs.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 24

3 years ago
Pushing to try again to see if it reproduces: https://treeherder.mozilla.org/#/jobs?repo=try&revision=325df11c4aaa
(Assignee)

Comment 25

3 years ago
The try I submitted earlier does not display the same leaks as those pointed to in comment #18, and I am unable to reproduce them locally, so let's try landing this again and see what happens.
Keywords: checkin-needed

Comment 27

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4a7597513280
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

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