Closed Bug 1274106 Opened 5 years ago Closed 5 years ago

Intermittent e10s devtools/client/aboutdebugging/test/browser_tabs.js | Test timed out

Categories

(DevTools :: about:debugging, defect, P3)

defect

Tracking

(e10s+, firefox49 wontfix, firefox50 fixed, firefox51 fixed, firefox52 fixed)

RESOLVED FIXED
Firefox 52
Tracking Status
e10s + ---
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: philor, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

Priority: -- → P3
Alex, it looks like your test timed out on Ubuntu once.
Flags: needinfo?(poirot.alex)
Summary: Intermittent browser_tabs.js | Test timed out → Intermittent devtools/client/aboutdebugging/test/browser_tabs.js | Test timed out
(In reply to Alexandre Poirot [:ochameau] from comment #2)
> I imagine there is bigger intermittent fish to fry.

No doubt there were, but right now, other than the js::ProfileEntry::pc crash this is the most-frequent devtools failure.
Summary: Intermittent devtools/client/aboutdebugging/test/browser_tabs.js | Test timed out → Intermittent e10s devtools/client/aboutdebugging/test/browser_tabs.js | Test timed out
Ok.

Looks like we never receive the DOM mutation:
 21:04:39     INFO -  36 INFO Entering test bound
 21:04:39     INFO -  37 INFO opening about:debugging
 21:04:39     INFO -  38 INFO Adding a new tab with URL: about:debugging#tabs
 21:04:39     INFO -  39 INFO Waiting for event: 'load' on [object XULElement].
 21:04:39     INFO -  40 INFO Got event: 'load' on [object XULElement].
 21:04:39     INFO -  41 INFO Tab added and finished loading
 21:04:39     INFO -  42 INFO Adding a new tab with URL: data:text/html,<title>foo</title>
 21:04:39     INFO -  43 INFO Waiting for event: 'load' on [object XULElement].
 21:04:39     INFO -  44 INFO Got event: 'load' on [object XULElement].
 21:04:39     INFO -  45 INFO Tab added and finished loading
 21:04:39     INFO -  46 INFO Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "data:text/html,<title>foo</title>" line: 0}]
21:04:39 INFO - 47 INFO TEST-UNEXPECTED-FAIL | devtools/client/aboutdebugging/test/browser_tabs.js | Test timed out - 

The two tabs, about:debugging and data: url are both opened, but the `yield onNewTab;` never resolves.
This is different from comment 2.
Assignee: nobody → poirot.alex
Blocks: e10s-tests
tracking-e10s: --- → +
I have a guess that using the openNewForegroundTab and removeTab functions of https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm might fix this.  I'll work up a patch and push it through try to test my theory.
Any luck? I'm willing to help here, but I can't reproduce locally and whenever I try to push some additional debug logs, it stops reproducing :x

If you are running out of time, do you want me to try replacing with BrowserTestUtils helpers?
Using BrowserTestUtils helpers didn't help by itself. 1 of 10 try attempts timed out.  I have another try run in progress that removes the waitForMutation where, as you pointed out above, `yield onNewTab;` never resolves. That try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f0a4db6d44f6e6d72389ab1d5e113608d007f1c
Oh wait, there is something wrong about the background tab. I didn't realiazed that before, but the tab isn't opened in background.
Here is a failure screenshot:
https://public-artifacts.taskcluster.net/Q-8khELfT626No7r_W0qJA/0/public/test_info//mozilla-test-fail-screenshot_NQzCBx.png
We clearly see it staying in foreground, I can easily imagine mutation event being paused if the tab isn't visible...
I also see the tab being in foreground when running locally.
Did your timeout was the same than the one we have here?
It looks like browser_tabs.js doesn't pull addTab from devtools/client/aboutdebugger/test/head.js, but another one... So if you modified addTab directly it wasn't taken into account.
Also, I don't see any way to open background tab in BrowserTestUtils??
here is the try log failure where I used BrowserTestUtils to open (openNewForegroundTab) and close (removeTab) the test tab: https://archive.mozilla.org/pub/firefox/try-builds/twalker@mozilla.com-a354dd723ef32c50851282a2b9c9cf4616ec0a8b/try-linux/try_ubuntu32_vm_test-mochitest-e10s-devtools-chrome-1-bm141-tests1-linux32-build49.txt.gz

I'm confused.  tab "foo" is in the foreground in the screen capture, is that not supposed to be the case?
We explicitely ask it to be a background tab here:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/test/browser_tabs.js#24
let newTab = yield addTab(TAB_URL, null, true);

Third argument to true:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/test/head.js#71

But it looks like this addTab helper is overloaded by some other one... which doesn't support such third argument. That may just be that.
ah, my mistake. my attempts to fix this then are really changing the test entirely from checking the background tab to checking a new foreground tab.  

Is it possible this !/devtools/client/framework/test/shared-head.js listed in browser.ini for this test directory, with it's own addTab function, could be the problem?
(In reply to Tracy Walker [:tracy] from comment #33)
> Is it possible this !/devtools/client/framework/test/shared-head.js listed
> in browser.ini for this test directory, with it's own addTab function, could
> be the problem?

Yes, most likely. Here is a try to fix that and reuse the one from shared-head:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d2a9bd3516a
I re-triggered M-e10s dt1 ten times in your try run. That is the chunk this test is in.  Let's see if any of them fail for this test.
Looks like you got it Alexandre.  10 and 10 more re-triggers with no failures in this test.
\o/ finally!

Julian, I'm removing addTab/removeTab from about:debugging's head.js as it was already overloaded by the one from shared-head.js. While doing that I'm backporting about:debugging features to the common one.
Comment on attachment 8791376 [details]
Bug 1274106 - Ensure opening tabs in background from about:debugging test.

https://reviewboard.mozilla.org/r/78800/#review77458

Great find, thank you for the investigation and fix.

::: devtools/client/aboutdebugging/test/head.js:66
(Diff revision 1)
>    document.querySelector(`[aria-controls="${panelId}"]`).click();
>    return waitForMutation(
>      document.querySelector(".main-content"), {childList: true});
>  }
>  
>  function closeAboutDebugging(tab, win) {

Remove unused win argument. Update browser_service_workers_not_compatible.js which was passing a win argument to this method.
Attachment #8791376 - Flags: review?(jdescottes) → review+
Comment on attachment 8791376 [details]
Bug 1274106 - Ensure opening tabs in background from about:debugging test.

https://reviewboard.mozilla.org/r/78800/#review77528
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8be679eee5cf
Ensure opening tabs in background from about:debugging test. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/8be679eee5cf
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Hum well, that wasn't easy to track down, but these new intermittents were highlighting a significant issue in about:debugging where tabs with the same name/title/url would be merged into a single one.

STR:
- open about:debugging
- open a new tab and load: data:text/html,fooo
- open another tab with the same url

You will see only two tabs in about:debugging, the two data:text/html,fooo are going to be merged :/
That ends up messing up with this test where the new tab will sometimes be merged with the first empty firefox tab.
Flags: needinfo?(poirot.alex)
Blocks: 1188981
I don't know why `key` usage has been introduced in bug 1188981,
but it was preventing tabs with the same title or url to be displayed
correctly in about:debugging.
Attachment #8792527 - Flags: review?(jdescottes)
Comment on attachment 8792527 [details] [diff] [review]
Allow displaying items with the same name/url/title in about:debugging

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

(In reply to Alexandre Poirot [:ochameau] from comment #49)
> Created attachment 8792527 [details] [diff] [review]
> Allow displaying items with the same name/url/title in about:debugging
> 
> I don't know why `key` usage has been introduced in bug 1188981,
> but it was preventing tabs with the same title or url to be displayed
> correctly in about:debugging.

The key property is apparently used by React to uniquely identify children of a list, and know which state should be assigned to which component (in case you the list and re-render for instance). All our target components are stateless, so I don't think this is needed here. (and if it was, it should allow to display two tabs targetting the same URL).

BTW, this will also solve Bug 1285962.
Attachment #8792527 - Flags: review?(jdescottes) → review+
Note that using a key can also avoid unnecessary re-renders from React, but given the lists we are displaying at the moment in about:debugging, this is not a must have. I will open follow ups to add tests covering the use cases you found here and reintroduce keys once we have proper test coverage.
https://hg.mozilla.org/integration/fx-team/rev/e682130bece76629eb0ca95908f8cc14b30d0f91
Bug 1274106 - Allow displaying items with the same name/url/title in about:debugging. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/e682130bece7
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(poirot.alex)
Comment on attachment 8792527 [details] [diff] [review]
Allow displaying items with the same name/url/title in about:debugging

Approval Request Comment
[Feature/regressing bug #]: bug 1188981
[User impact if declined]: any item in about:debugging (tabs, service workers, addons) with duplicated name are going to merged into just one item.
[Describe test coverage new/current, TreeHerder]: this is the main reason of this uplift. uplift that small fix to prevent various intermittent.
[Risks and why]: small change into about:debugging, restricted to about:debugging but affect all its panels.
[String/UUID change made/needed]: no

The second patch is test only and could be uplifted with that one, but it needs this patch to really get rid of this intermittent.
Flags: needinfo?(poirot.alex)
Attachment #8792527 - Flags: approval-mozilla-beta?
Attachment #8792527 - Flags: approval-mozilla-aurora?
Comment on attachment 8792527 [details] [diff] [review]
Allow displaying items with the same name/url/title in about:debugging

Fixes a regression in about:debugging found by an intermittent test failure, Aurora51+, Beta50+
Attachment #8792527 - Flags: approval-mozilla-beta?
Attachment #8792527 - Flags: approval-mozilla-beta+
Attachment #8792527 - Flags: approval-mozilla-aurora?
Attachment #8792527 - Flags: approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.