Closed Bug 1344087 Opened 7 years ago Closed 7 years ago

The API that browser_ext_sessions_getRecentlyClosed_tabs.js tests might be broken

Categories

(WebExtensions :: Untriaged, enhancement, P3)

enhancement

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mossop, Assigned: bsilverberg)

References

Details

(Whiteboard: [sessions]triaged)

Attachments

(1 file)

Over in bug 1337627 I was hitting problems with browser_ext_sessions_getRecentlyClosed_tabs.js loading about:addons and then closing it without waiting for initialization to complete so I was switching it to a different page. A few other pages I tried found errors:

about:buildconfig made the test fail with an unexpected title for the page (Got null, expected about:buildconfig).
http://example.com/ made the test fail with an unexpected favicon (Got http://example.com/favicon.ico, expected null).

I didn't see anything obviously broken in the test but didn't want to dig too deep so I just found another page that works to unblock me, but my suspicion is that the API that this is testing might be broken in some subtle way.
Thanks for reporting this, Dave. I will look into it.
Assignee: nobody → bob.silverberg
Whiteboard: investigate
Priority: -- → P3
Whiteboard: investigate → [sessions]triaged
I have looked into this and there are two problems here.

1. The test is not waiting long enough for the favicon to resolve before storing the information to be used later in an assertion. This didn't matter with the url currently being used in the test, but does manifest when using http://example.com/.  I will fix this in the test and use that url to prove it is fixed.

2. There is an issue with SessionStore that manifests when a page has a title that is the same as the url (e.g., about:buildconfig). The title is not stored by SessionHistory, which means that when the data is returned by SessionStore the title is missing. This _could_ be fixed in the sessions API by using that assumption to populate the title, but I would rather fix this in SessionStore. I am currently discussing the fix for that with Mike deBoer and will open a separate bug for that once we decide what to do about it.
Depends on: 1344857
I've added a patch to this bug which fixes the timing problem in the test. I'm going to hold off requesting review of that until bug 1344857 lands though, as the test also depends on that now.
Comment on attachment 8857078 [details]
Bug 1344087 - Update browser_ext_sessions_getRecentlyClosed_tabs.js to use a URL with an identical title,

https://reviewboard.mozilla.org/r/128976/#review132598
Attachment #8857078 - Flags: review?(mixedpuppy) → review+
Hrm, adding to the timeout...do you know what part is taking that long?
Flags: needinfo?(bob.silverberg)
It's the whole damn test! I already had to add that to browser_ext_sessions_getRecentlyClosed.js [1], and I think my change to the test that is making it wait a bit longer is what has caused this to go intermittent on try with a timeout. It's only on certain platforms and debug builds, and I think all the opening and closing of windows is just really slow there. If you look at the test you'll see it isn't actually doing a lot of different things, so there's no logical place to split it up.

If you have any suggestions I'd be happy to try, but barring that I'm not sure what else to do than request a longer timeout.

[1] http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed.js#7
Flags: needinfo?(bob.silverberg)
Actually, looking more closely at the try run, I think something else is going on. None of the test seems to be running before the timeout, so maybe there's a problem with the new code I added which opens the first tab. I'll look into that further next week.
Comment on attachment 8857078 [details]
Bug 1344087 - Update browser_ext_sessions_getRecentlyClosed_tabs.js to use a URL with an identical title,

https://reviewboard.mozilla.org/r/128976/#review132672

::: browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed_tabs.js:77
(Diff revision 6)
>      checkTabInfo(expectedTabs[x], tabInfos[x]);
>    }
>  
>    await extension.unload();
>  
>    // Test without tabs permission.

This should probably be in its own add_task
(In reply to Bob Silverberg [:bsilverberg] from comment #13)
> Actually, looking more closely at the try run, I think something else is
> going on. None of the test seems to be running before the timeout, so maybe
> there's a problem with the new code I added which opens the first tab. I'll
> look into that further next week.

Yeah, I took a look at the logs and saw a bunch of stuff that seems potentially unrelated, it could be a prior test causing issues.
Comment on attachment 8857078 [details]
Bug 1344087 - Update browser_ext_sessions_getRecentlyClosed_tabs.js to use a URL with an identical title,

https://reviewboard.mozilla.org/r/128976/#review132672

> This should probably be in its own add_task

I'm not sure that makes sense to do. The tests here rely on all of the tabs that have been opened and closed above, as well as the data we stored for those tabs - basically the code from line 39 - 51 in the test, so just moving this code into its own task won't work. I could break it into two tasks, and do all the work from lines 39 - 51 in each task, but considering that that code is what is causing the test to take so long to run, it seems like that would create more problems than it solves. Am I misunderstanding your suggestion, or does what I'm saying make sense?
I've spent a lot of time trying to get this test passing consistently on try, when using http://example.com as the first tab. There are issues with waiting for BrowserTestUtils.browserStopped, which seems to work locally for me, but fails pretty much everywhere on try. I believe the issue is also caused by a quirk with favicon.ico which causes a default one to be reported when there is not actually a favicon.ico defined for the page, which then causes my assertion to fail.

I only added this to the test because this bug originally talked about how the test started to fail when trying to use http://example.com/, so I thought it worthwhile to fix that case, but that's really just an issue with the test, and there is no requirement to run this test with that URL loaded into a tab. The test is perfectly valid with only about: URLs loaded, so I don't think it's worth any more of my time to try to solve this problem when it doesn't actually make the test any better.

I did update the test to use about:buildconfig as that did uncover an issue which I have since fixed.

Shane, what do you think? Also, you never replied to my question in comment 16, although I think it's now moot.
Flags: needinfo?(mixedpuppy)
If that fixes it, cool, r=me  OP suggested that using about:buildconfig failed the test, is that an issue?
Flags: needinfo?(mixedpuppy)
(In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> If that fixes it, cool, r=me  OP suggested that using about:buildconfig
> failed the test, is that an issue?

No, that's what I fixed via bug 1344857, and I've updated this test to use about:buildconfig so we have coverage for that.
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2e1139a478b4
Update browser_ext_sessions_getRecentlyClosed_tabs.js to use a URL with an identical title, r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/2e1139a478b4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: