Closed Bug 1691575 Opened 3 years ago Closed 2 years ago

Fix and re-enable browser/components/extensions/test/browser/browser_ext_devtools_network.js for Fission

Categories

(WebExtensions :: Developer Tools, defect, P3)

Product:
WebExtensions
Component:
Developer Tools
Type:
defect

Tracking

(Fission Milestone:MVP, firefox92 fixed)

Status:
RESOLVED FIXED
Milestone:
92 Branch
Project Flags:
Fission Milestone MVP
Tracking Flags:
Tracking Status
firefox92 --- fixed

People

(Reporter: cpeterson, Assigned: nchevobbe)

References

(Blocks 3 open bugs)

Details

(Whiteboard: dt-fission-m3-mvp)

Attachments

(2 files)

Bug 1691575 - [devtools] Don't explicitly destroy target front in head_devtools.js closeToolboxForTab. r=ochameau.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1691575 - [devtools] Don't rely on webconsole fronts to fetch longStrings fullText. r=bomsy.
48 bytes, text/x-phabricator-request
Details | Review

@ Tomislav, here's a webext test disabled for Fission. Should this bug block shipping Fission MVP? This test looks pretty flaky since it's also skipped for all Linux, debug macOS, and debug Win64.

This test was disabled for Fission in bug 1597673.

https://searchfox.org/mozilla-central/rev/7067bbd8194f4346ec59d77c33cd88f06763e090/browser/components/extensions/test/browser/browser.ini#108-109

[browser_ext_devtools_network.js]
skip-if = fission || os == 'linux' || (os == 'mac' && debug) || (debug && os == 'win' && bits == 64) # Bug1570478
Flags: needinfo?(tomica)
Blocks: fission-tests

Luca pointed at bug 1694906 and specifically D107100 that updates this test, and should investigate after that lands.

Flags: needinfo?(tomica)
Severity: -- → S3
Priority: -- → P3
Depends on: 1694906
Fission Milestone: M7 → M7a

I did look a bit into what is currently preventing this test to pass on fission, and the test file does still fail, but not with the same error that was mentioned in Bug 1597673 comment 0, which was likely [JavaScript Error: "TypeError: toolbox is undefined" {file: "resource://devtools/shared/fronts/targets/local-tab.js" line: 155}]).

On a recent mozilla-central tip:

  • the first test case (test_devtools_network_on_navigated) does instead pass with both fission enabled and disabled.

  • the second test task (test_devtools_network_get_har) does now fail because of the following assertion failure:

Unexpected Results
------------------
browser/components/extensions/test/browser/browser_ext_devtools_network.js
  FAIL HAR log should not be empty - Got +0, expected 1
Stack trace:
chrome://mochikit/content/browser-test.js:test_is:1359
chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_devtools_network.js:test_devtools_network_get_har:209
  • the third test case (test_devtools_network_on_request_finished) does fail for a timeout (it seems it the event browser.devtools.network.onRequestFinished is not fired while the test runs with fission enabled).

I doubt that Bug 1694906 will change the behavior of the two tests that are currently failing and make them to pass.
But I see we do have a meta related to changes needed to make the netmonitor Fission-ready: Bug 1578721
and some of the issue stacked by that meta are not yet closed.

By Comparing what seems to behave differently when the test run with fission and without fission enabled in the test_devtools_network_get_har, it seems to me that the underlying issue is more likely related to the behaviors of the netmonitor internals, which seems to be different while running in the two modes and that breaks the assumptions that the test is doing:

If I tweak the test to skip the assertion on the getHAR result for the tab being navigated between two different domains, then the test does pass (because reloading the tab does still behave in the same way and the getHAR results are the expected ones in both the modes).

And so it looks that NetMonitorAPI is not collecting the expected request when we are navigating between two different TLD+1, which with Fission enabled does trigger process switch, while it doesn't when Fission is disabled.

(I haven't digged into the reasons behind the timeout of the last test, but it is unrelated to the getHAR issue described above).

Hi Hubert,
would you mind to help us figuring out this issue? (see comment 2 and comment 3 for some details from what I observed)

In particular I was also wondering:

  • does the different behavior with and without fission sound like something that may already be tracked by one of the issue that are already blocking Bug 1578721?
  • is the different behavior something that we plan to fix or is something that is expected and it is not going to change? (if that is the expected behavior with Fission then we may have to opt for changing the expectations of the test)

Feel free to ping me on matrix or slack if discussing briefly about this synchronously may be helpful.

Thanks in advance!

Flags: needinfo?(hmanilla)

Hi Luca,

Thanks for looking into this. Your analysis is on point

  • does the different behavior with and without fission sound like something that may already be tracked by one of the issue that are already blocking Bug 1578721?

This might relate to a target-switching issue which restricts us enabling a couple of other tests see https://bugzilla.mozilla.org/show_bug.cgi?id=1601331#c21 or might just be something else in the netmonitor. I'll have an in-depth look in a bit to clearly distinguish.

  • is the different behavior something that we plan to fix or is something that is expected and it is not going to change? (if that is the expected behavior with Fission then we may have to opt for changing the expectations of the test)

This will be clear once i've had a look.

Thanks again!

Flags: needinfo?(hmanilla)

Adding back the needinfo for me to do more investigations.

Flags: needinfo?(hmanilla)

Hi Luca,
I did some more investigations and it looks like this relates to the target switching i issue mentioned in the link in Comment 5 above. It blocks a couple of devtools tests from being enabled as well.
The issue occurs mostly when switching between targets of different domains. Details are discussed here https://bugzilla.mozilla.org/show_bug.cgi?id=1644397#c5

So to answer your question

  • is the different behavior something that we plan to fix or is something that is expected and it is not going to change? (if that is the expected behavior with Fission then we may have to opt for changing the expectations of the test)

It is not the expected behavior, and there is a bug and plan to fix it. See Bug 1644397

Thanks a lot.

Depends on: 1644397
Flags: needinfo?(hmanilla)

Hi Hubert,
thanks a lot for looking into the devtools bugzilla issue that is tracking that work and adding it as a dependency of this issue, very much appreciated!

I just ran the test and it still fails with:

 0:14.46 INFO Wait the devtools page load
 0:15.06 GECKO(109352) console.error: (new TypeError("can't access property \"enableActions\", this.dataProvider is undefined", "resource://devtools/client/netmonitor/src/connector/index.js", 298))
 0:15.06 GECKO(109352) JavaScript error: resource://devtools/client/netmonitor/src/api.js, line 138: TypeError: can't access property "log", har is null
 0:15.06 INFO Console message: [JavaScript Error: "TypeError: can't access property "log", har is null" {file: "resource://devtools/client/netmonitor/src/api.js" line: 138}]
 0:15.37 PASS onNavigated received the expected url. - Expected: http://example.com/, Actual: http://example.com/ - 
 0:15.37 INFO Wait for an onRequestFinished event

It also fails when enabling server side target switching (devtools.target-switching.server.enabled):

 0:09.70 GECKO(111393) console.error: (new InvalidStateError("JSWindowActorChild.sendAsyncMessage: JSWindowActorChild cannot send at the moment", (void 0), 389))
 0:09.70 GECKO(111393) InvalidStateError: JSWindowActorChild.sendAsyncMessage: JSWindowActorChild cannot send at the moment: sendPacket@resource://devtools/server/connectors/js-window-actor/DevToolsFrameChild.jsm:389:10
 0:09.70 GECKO(111393) send@resource://devtools/shared/transport/js-window-actor-transport.js:53:25
 0:09.70 GECKO(111393) send@resource://devtools/server/devtools-server-connection.js:99:20
 0:09.70 GECKO(111393) _sendEvent@resource://devtools/shared/protocol/Actor.js:72:15
 0:09.70 GECKO(111393) initialize/<@resource://devtools/shared/protocol/Actor.js:46:16
 0:09.70 GECKO(111393) _emit@resource://devtools/shared/event-emitter.js:226:34

Hubert, This test still fails, would you mind having a look again?

Flags: needinfo?(hmanilla)
Assignee: nobody → hmanilla
Flags: needinfo?(hmanilla)
Whiteboard: dt-fission-m3-triage

So i did a bit more investigations, and this relates to a target-switching issue with willNavigate which clears the requests too late so early requests which belong to the new page get removed. So the state.requests.requests expected here have already been cleared.

This should be fixed when Bug 1706995 lands.

Depends on: 1706995
Fission Milestone: M7a → ---
Whiteboard: dt-fission-m3-triage

(In reply to Hubert Boma Manilla (:bomsy) from comment #11)

This should be fixed when Bug 1706995 lands.

In that case, I will track this test bug for Fission M8 milestone like Bug 1706995.

Fission Milestone: --- → M8
Depends on: 1712592
No longer depends on: 1706995

Adding dt-fission whiteboard tag to note that this is a DevTools bug and doesn't need to block shipping Fission MVP.

Whiteboard: dt-fission

Hi Hubert and Alex,
I looked a bit into the behavior of this test with Fission (Bug 1706995 seems to have been landed and I was looking into bug 1570478) and it is still failing for me when I have execute it locally, but the underlying issue seems to be a different one now.

It is not a permafailure, it is failing intermittently and only in chaos mode for me locally, likely for the race that I'm going to describe in a bit more detail below:

  • The STR to reproduce the failure locally (at least for me locally on linux) is:
MOZ_CHAOSMODE=1 mach mochitest browser/components/extensions/test/browser/browser_ext_devtools_network.js --verify --enable-fission

  • The MOZ_CHAOSMODE environment variable is only needed to get a failure in a shorter amount of time, it would get one also without it but only at the end while running the test again in chaos mode.

  • The test file is getting stuck in test_devtools_network_on_request_finished, while waiting the browser.devtools.network.onRequestFinished listener to have been called

  • when the test does get stuck the underlying reason is that we throw a can't access property "log", har is null exception from netmonitor's onPayloadReady

  • by digging a bit more into what was making har to be null:

If I'm not mistaken, in practice these conditions may only happen if firefox-data-provider has called targetFront.getFront("console") after the targetFront was cleaned up, which may me realize that this looks a lot like a race happening while switching the target on a process switch.

No need to rush, but I wanted to describe you what I observed and hear what is your perspective on the issue and rationale described above, and what would be a reasonable way to fix the underlying issue properly
(I do suspect that we may want to refactor some of those netmonitor and webconsole internals to better fit the new descriptors & commands API design, but that is mainly "a guess" ;-)).

Flags: needinfo?(poirot.alex)
Flags: needinfo?(hmanilla)

This bug is assigned to Fission Milestone M8, but has whiteboard tag dt-fission instead of dt-fission-m3-mvp.

I'm adding the dt-fission-m3-triage tag so DevTools Fission triage can decide whether this bug should be fixed for dt-fission-m3-mvp or can be postponed from Fission Milestone MVP to Future.

Whiteboard: dt-fission → dt-fission-m3-triage

I tested locally with patches from Bug 1713607 - Clear the console in case of bfcache navigations
but it still fails, timeout on:
0:19.14 INFO Wait for an onRequestFinished event

I was testing with SSTS enabled.
$ mach test browser/components/extensions/test/browser/browser_ext_devtools_network.js --enable-fission --setpref devtools.target-swit ching.server.enabled=true

Honza

Fission Milestone: M8 → MVP
Whiteboard: dt-fission-m3-triage → dt-fission-m3-mvp
Assignee: hmanilla → nchevobbe
Status: NEW → ASSIGNED

The helper is already calling gDevTools.closeToolboxForTab, which does take care
of destroying its associated descriptor.
This makes some extension test to pass with server side target switching enabled.

The Netmonitor used to call the WebConsoleFront getString method to get the full
text of a long string. This could make some test to fail with Fission and target
switching.
Since the webconsole actor is not even involved in the webconsole method, there's
no reason to keep using it. A similar function is added into a util file; the only
difference is that it doesn't cache the front it creates, which does not matter
as the netmonitor only calls this once and then put the full text in its redux
store.
This allows us to re-enable browser_ext_devtools_network.js with Fission.

Depends on D120332

Blocks: 1721375
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/83d6172883e9
[devtools] Don't explicitly destroy target front in head_devtools.js closeToolboxForTab. r=ochameau,rpl.
https://hg.mozilla.org/integration/autoland/rev/e30b2a84440d
[devtools] Don't rely on webconsole fronts to fetch longStrings fullText. r=bomsy.

https://hg.mozilla.org/mozilla-central/rev/83d6172883e9
https://hg.mozilla.org/mozilla-central/rev/e30b2a84440d

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox92: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 92 Branch
Flags: needinfo?(hmanilla)
Flags: needinfo?(poirot.alex)
You need to log in before you can comment on or make changes to this bug.