Closed Bug 1455796 Opened 2 years ago Closed 2 years ago

Intermittent dom/base/test/browser_messagemanager_loadprocessscript.js | Should get back to the base number of processes at this point (3 or 4) -

Categories

(Core :: DOM: Core & HTML, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox61 + fixed
firefox62 --- fixed

People

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

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:other])

Attachments

(1 file)

There are 37 failures asociated to this bug in the last 7 days. This is occurring mostly on Windows 7 and 10 pgo and opt builds.

:overholt can you take a look at this?
Flags: needinfo?(overholt)
Whiteboard: [stockwell needswork:owner]
This is triggered by the landings of bug 1386807. It got filed for the initial landing, disappeared when it got backed out and returned with a high frequency when it relanded and stuck.

Ryan, shall one wait for a fix or back bug 1386807 out? The latter would cause the WebCompat system addon to miss Firefox 61.
Flags: needinfo?(ryanvm)
Flags: needinfo?(dschubert)
It looks like this is a timing issue related to out-of-process webextensions (bug 1190679), which would explain why we only have windows and osx failures, but no Linux failures.

Quickly glancing over the test, I saw we already added some wiggle-room for activity-streams, see bug 1372664. My assumption (only verified by running two runs) is that the WebExtension in my patch adds one new process by the time the test runs, which makes ActivityStreams being the cause of unpredictability again. Simply increasing the `BASE_NUMBER_OF_PROCESSES` by one of OOP WebExts are enabled is not a viable solution, since the process will only spawn if a WebExt is actually loaded, so it could be another cause for intermittent fails if the webextension does not load fast enough. 

In any case, I'm on PTO and only can have a more detailed look at this on Wednesday, sorry. Just wanted to quickly drop some initial thoughts. Leaving the ni? active so I can come back to this.
I think Dennis' assessment sounds believable. Given that this is more of a test robustness issue (that we were likely to hit eventually anyway as more built-in extensions move to webext) and not pointing to a specific issue with the WebCompat addon, I think we can live with this for now until he returns.
Flags: needinfo?(ryanvm)
Assignee: nobody → dschubert
Flags: needinfo?(overholt)
So, I have this patch that makes the test aware of processes used for running WebExtensions. Initially, I wanted to completely get rid of the tolerances added for activity stream, but given the discussions in bug 1372664, this is probably really hard and I was not able to come up with a solution. Given the time pressure here, it's probably best to leave it as is.

I did a generic try push at [0] to make sure these changes do not break some systems, and one run with 10 retriggers for that test in Winwodws 10 x64 pgo, as we have the most failures there: [1]. Both pushes are green, so this should be fine!

Peter, as you recently worked on MessageManagers, do you mind having a look at these test changes? Would be highly appreciated. :)

[0]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8f8ecb29d04aadd6c8ef79fef673416c0b7d657
[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=757371bf9a48c0f1eec4548e24f3426da6648ec2&filter-searchStr=bc2&filter-resultStatus=success&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Flags: needinfo?(dschubert)
Flags: needinfo?(peterv)
Hey Blake, since you're away on IRC and it's getting late for me, I'll try via ni?. :)

This bug has a patch attached that's making the test in question aware of the extra process used for WebExtensions. It's not a complex change, but as this is causing an intermittent with high failure rates in beta, I'd appreciate a fast review. Peter seems to be busy, do you think you have time to quickly review my changes?
Flags: needinfo?(mrbkap)
Flags: needinfo?(overholt)
Comment on attachment 8972586 [details]
Bug 1455796 - Make browser_messagemanager_loadprocessscript.js aware of OOP WebExtensions

https://reviewboard.mozilla.org/r/241154/#review248190

::: dom/base/test/browser_messagemanager_loadprocessscript.js:10
(Diff revision 1)
> +
> +  // If we run WebExtensions out-of-process (see bug 1190679), there might be
> +  // additional processes for those, so let's add these to the base count to
> +  // not have system WebExtensions cause test failures.
> +  for (let i = 0; i < Services.ppmm.childCount; i++) {
> +    if (Services.ppmm.getChildAt(i).remoteType === "extension") {

Depending on whether you can get at it here, it might make sense to use E10SUtils.EXTENSION_REMOTE_TYPE.
Attachment #8972586 - Flags: review?(peterv) → review+
Flags: needinfo?(peterv)
Flags: needinfo?(overholt)
Flags: needinfo?(mrbkap)
Ah, didn't know this was a thing! Thanks much for the note, and for the review. Adjusted the test to use E10SUtils.EXTENSION_REMOTE_TYPE.

Running a quick try run to make sure E10SUtils is available on all systems, and not only on my machine. It should be, but just to be sure... https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9b990acfe8f8a11c79a519996b6d12e51aabecc Will mark for landing once the try run is green.

Dropping all ni?s as we now have a review. (Actually, :overholt was faster. Heads-on collision! :))
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e4cfb8924eea
Make browser_messagemanager_loadprocessscript.js aware of OOP WebExtensions r=peterv
Keywords: checkin-needed
Priority: -- → P1
Comment on attachment 8972586 [details]
Bug 1455796 - Make browser_messagemanager_loadprocessscript.js aware of OOP WebExtensions

This is not yet merged into central, but let's get beta approval anyway, as this is the number #2 intermittent!

[Feature/Bug causing the regression]: Bug 1386807 
[User impact if declined]: None, but we'd have an intermittent in beta.
[Is this code covered by automated tests?]: N/A
[Has the fix been verified in Nightly?]: No, but the test passes on try and autoland.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's a test-only patch. :)
[String changes made/needed]: None.
Attachment #8972586 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/e4cfb8924eea
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment on attachment 8972586 [details]
Bug 1455796 - Make browser_messagemanager_loadprocessscript.js aware of OOP WebExtensions

This is a test-only change, so no approval is required.
Attachment #8972586 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/releases/mozilla-beta/rev/784fa857364d
Whiteboard: [stockwell disable-recommended] → [stockwell fixed:other]
Component: DOM → DOM: Core & HTML
See Also: → 1588313
You need to log in before you can comment on or make changes to this bug.