Closed Bug 1378524 Opened 7 years ago Closed 7 years ago

Intermittent unknown test url | uncaught exception - TypeError: tab is null at createTooltip@chrome://browser/content/tabbrowser.xml:5073:1

Categories

(Firefox :: Tabbed Browser, defect, P3)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox57 --- disabled
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: rpl)

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Bug 1378524 - Fix "TypeError: tab is null" exception raised from browser_inspector_extension_sidebar.js when running on beta.
59 bytes, text/x-review-board-request
pbro
: review+
Details 
See https://wiki.mozilla.org/Bugmasters/Projects/Bug_Handling/Bug_Husbandry#Intermittent_Test_Failure_Cleanup
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
This is extremely frequent on Beta. We really need to tone down this bot if it's going to close bugs out this aggressively.
Status: RESOLVED → REOPENED
Flags: needinfo?(ehumphries)
Resolution: INCOMPLETE → ---
On a related note, can you help me find an owner for this bug, Florian?
Flags: needinfo?(florian)
Given the timing of the spike, I wonder if bug 1399454 is involved.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)
> On a related note, can you help me find an owner for this bug, Florian?

This is devtools tests, I don't know much about them, and it's not clear which test is failing here, so I don't really know how to help.
Flags: needinfo?(florian)
Luca, can you please look at this? It's always happening on shutdown after browser_inspector_extension_sidebar.js runs. Linux-only near-permafail. Though this test hasn't been touched in over a week, so I wonder if something from extensions land made this angry.
Flags: needinfo?(lgreco)
Sorry, first instances on Beta actually correspond to this range on m-c:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=30a386ff1192&tochange=e4261f5b96eb
:ryanvm the batch job to update intermittent test bug failures needs to move from Thursday to Sunday night after the Orange Factor bot's run. Right now we're closing bugs that haven't had new comments in 21 days, should that be longer?
Flags: needinfo?(ehumphries)
I'm able to reproduce this issue locally by running the test file (devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js) on a beta artifact build:

The issue doesn't happen on every test execution, but it happens pretty often, I've added some additional logging to be able to identify which is the tooltip that is failing to be created, and the tooltip that is raising the exception is "tabbrowser-tab-tooltip", defined in browser.xul:

- http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/browser/base/content/browser.xul#465

That tooltip should be the one that appears when the mouse is hovering a tab, given that the exception is raised while the test is shutting down (and both the tab and the toolbox opened for the test have been closed, and the first empty tab automatically opened when the test has been started is also closed, or being closed), the failure seems to be due to the tooltip being created too late.

I briefly looked at the changes included in the push log linked in Comment 11, but I've not yet found which change could have introduced this failure (or increased the chances of creating the tab tooltip after a tab has been closed), but in the meantime I've pushed to try the patch that I've just attached to this issue (which exits the `createTooltip` tabbrowser method if the document.tooltipNode is null or undefined, in addition to exist the method is the document.tooltipNode is not a tab):

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=5339004fb3df3eaa96a0a8808451513badf524d7

Another interesting detail that is worth a mention:

this test file does not explicitly open a new tab on its own, on the contrary these tabs are opened implicitly when the test is starting (the first empty tab) and then, while the test is running, by the `openInspectorForURL` test helper (http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/devtools/client/inspector/test/head.js#140) which opens a tab the `addTab` shared test helper, http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/devtools/client/framework/test/shared-head.js#139) and then all the tabs and toolbox are closed by a function registered on cleanup: 

- http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/devtools/client/framework/test/shared-head.js#123-127

And so I would expect this failure to happen also for other tests which are using the same test helpers, but I'm not seem the failure happening for the other similar inspector tests.

Also, it doesn't seem that the issue can be reproduced on mozilla-central, or at least I've never been able to reproduce it yet.
I've been verified (first locally, then also on try) that the sidebar test is not the only one that can fail by raising that exception, but also other inspector tests that are using the same test helpers do fail when they run alone,
e.g. in the following try push:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5af1f1ba5a7e64414ed93567bb2af37ce0115a1&selectedJob=133170957

The test that is failing because of the "tab is null" exception is devtools/client/inspector/grids/test/browser_grids_restored-after-reload.js and the only change applied on top of the beta tip is the following patch:

- https://hg.mozilla.org/try/rev/3b98fe8ae14448a253db60389f38401a6b283b0c

which only add "skip-if = true" to all the inspector grid test files but one.

There is currently only one test file in the devtools inspector sidebar tests directory and it could be the reason why it is the only one that "seems" to be affected by this issue on beta, but the underlying issue seems to be there for all the other similar tests (locally I also tried on some of the other similar inspector test files with the same result)
Flags: needinfo?(lgreco)
Comment on attachment 8911754 [details]
Bug 1378524 - Fix "TypeError: tab is null" exception raised from browser_inspector_extension_sidebar.js when running on beta.

I'm marking this patch as obsolete for the following reasons:

- as described in comment 18, I've verified that the issue is not triggered by something different that the inspector's extension sidebar test is doing itself, on the contrary it seems to be related to the test helpers shared with the other inspector tests, but the issue seems to be triggered only if the test is executed alone (e.g. by being the only one in a test directory like the extension sidebar test file)

- if we just prevent the exception to be raised, we are going to cure the symptoms (the test failure), but not the underlying issue (because with or without the exception I've verified locally that an empty tooltip is shown near the tab strip)

Nevertheless, in the meantime this test has still an high rate of failures on beta and it is pretty annoying, I've one additional test file which also tests the extension sidebar (http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_devtools_panels_elements_sidebar.js), and so (if :gl is also ok with it) I would be ok if we decide to temporarily disable this test file on beta (the same sidebar test file doesn't seem to have this issue on Nightly) and file a separate bugzilla issue related to investigate and solve the underlying issue.
Attachment #8911754 - Attachment is obsolete: true
Priority: -- → P3
FYI, this is still showing up frequently in the 58-as-Beta uplift simulations on Try. Any chance we can move this bug forward so we're not left disabling it again for another cycle?
Flags: needinfo?(lgreco)
Hi Ryan
I took a deeper look at the shared test helpers (which are the actual reason for the failure, as briefly described in Comment 17 and Comment 18) to identify which is the piece that is actually introducing the issue.

The underlying issue is the following:

- when the test is exiting, the shared helpers are going to close all the opened tabs
- when the test helpers are closing the last opened tab (which is an empty about:blank tab), 
  a tooltip is showing while the same tab is closing.

My guess was that there is something in the test helpers that is forcing the mouse to be positioned on the tab that is closing, and after digging inside the shared test helpers I've finally found where is the code that is forcing the mouse position while the test is exiting. 

The following piece of code (registered to be executed when the test is exiting using `registerCleanupFunction`)
is where the mouse is being positioned over the tab (and triggering the tab tooltip to be opened):

- https://searchfox.org/mozilla-central/rev/fe75164c55321e011d9d13b6d05c1e00d0a640be/devtools/client/inspector/test/head.js#44-50
Flags: needinfo?(lgreco)
Comment on attachment 8911754 [details]
Bug 1378524 - Fix "TypeError: tab is null" exception raised from browser_inspector_extension_sidebar.js when running on beta.

Hi Gabriel,
As described in Comment 30 the reason for this intermittent failure is this line in the shared head.js:

- https://searchfox.org/mozilla-central/rev/fe75164c55321e011d9d13b6d05c1e00d0a640be/devtools/client/inspector/test/head.js#44-50

This patch currently workarounds the issue by moving the mouse in a screen position that is not "1,1" (which is basically over the tab and triggers the tooltip for the tab to be shown while we are closing the same tab).

I'm far from thrilled by the current fix in this patch, and I would feel better if we could fix it in the head.js instead, but I've attached the patch so that it is easier to reproduce the issue and check that moving the mouse in a different position fixes the intermittent failures.

Based on the inline comment, it seems that "1,1" was an arbitrary position which ensures that the mouse was out of the inspector, and so I was tempted to change it to something that is still out of the inspector, but it is also not over the tab which is going to be closed when the test is completed.

Let me know what do you think about this issue and if you have any advice/ideas, so that I can tweak the patch to fix the issue for this test and also prevent the same issue to appear on other inspector tests.
Attachment #8911754 - Flags: feedback?(gl)
Comment on attachment 8911754 [details]
Bug 1378524 - Fix "TypeError: tab is null" exception raised from browser_inspector_extension_sidebar.js when running on beta.

https://reviewboard.mozilla.org/r/183152/#review205806

::: devtools/client/inspector/extensions/test/browser_inspector_extension_sidebar.js:11
(Diff revision 2)
>  
> +registerCleanupFunction(function* () {
> +  // Move the mouse so that it is not positioned on the tab,
> +  // to prevent a failure related to a tooltip that is showing while
> +  // the same tab is being closed (See Bug 1378524 Comment 30 for rationale).
> +  EventUtils.synthesizeMouseAtPoint(100, 100, {type: "mousemove"}, window);

I don't think we should do this in registerCleanUpFunction. We sometimes have to move the mouse point at the end of a test. I would move this function call to where it is appropriate along with the comment where it is appropriate in the end of the test.
Attachment #8911754 - Flags: feedback?(gl)
Comment on attachment 8911754 [details]
Bug 1378524 - Fix "TypeError: tab is null" exception raised from browser_inspector_extension_sidebar.js when running on beta.

https://reviewboard.mozilla.org/r/183152/#review205806

> I don't think we should do this in registerCleanUpFunction. We sometimes have to move the mouse point at the end of a test. I would move this function call to where it is appropriate along with the comment where it is appropriate in the end of the test.

I do agree, unfortunately the code that moves the mouse in the "1,1" position is using registerCleanUpFunction (the additional was more a way to test that the original "mousemove" from https://searchfox.org/mozilla-central/rev/fe75164c55321e011d9d13b6d05c1e00d0a640be/devtools/client/inspector/test/head.js#44-50 is triggering the issue that makes this test to fail intermittently, than an actual fix).

I chatted with Patrick to know more about the reasons behind the original workaround from the shared head.js (and it should be because of the inspector's highlighters tests).

Unfortunately I've not been yet able to find a better way to fix it than moving the mouse in a different position, one that should be as good as the original "1,1" position for the original issue (on the highlighters tests), and it is also not going to trigger the tab tooltip (e.g. the top-right corner of the window) that makes the inspector extension sidebar test to fail (and that it would also make to fail any new test file that is the only test file in a new test dir, as described in Comment 18).
Attachment #8911754 - Flags: review?(pbrosset)
Comment on attachment 8911754 [details]
Bug 1378524 - Fix "TypeError: tab is null" exception raised from browser_inspector_extension_sidebar.js when running on beta.

https://reviewboard.mozilla.org/r/183152/#review206328
Attachment #8911754 - Flags: review?(pbrosset) → review+
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/ed872853ea60
Fix "TypeError: tab is null" exception raised from browser_inspector_extension_sidebar.js when running on beta. r=pbro
https://hg.mozilla.org/mozilla-central/rev/ed872853ea60
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Assignee: nobody → lgreco
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: