Closed Bug 1261185 Opened 4 years ago Closed 4 years ago

Complete test coverage for browser.windows.create

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.3 - Apr 25
Tracking Status
firefox48 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

Attachments

(1 file)

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: 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.
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.
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)
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|
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)
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.
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.
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+
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/
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)
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)
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
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)
Pushing to try again to see if it reproduces: https://treeherder.mozilla.org/#/jobs?repo=try&revision=325df11c4aaa
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
https://hg.mozilla.org/mozilla-central/rev/4a7597513280
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.