1285886 - [e10s-multi] Multiple content processes generates unexpected tabs.onUpdated events.

Status: RESOLVED WORKSFORME

(WebExtensions :: Untriaged, defect, P3)

Description

[Gabor Krizsanits (INACTIVE)]

Running web extension tests with multiple content processes I've got:

57 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js | got unexpected number of updateInfo data - Expected: [{"status":"loading"},{"status":"loading","url":"http://mochi.test:8888/browser/browser/components/extensions/test/browser/context_tabs_onUpdated_page.html"},{"status":"complete"}], Actual: [{"status":"loading"},{"status":"complete"},{"status":"loading"},{"status":"loading","url":"http://mochi.test:8888/browser/browser/components/extensions/test/browser/context_tabs_onUpdated_page.html"},{"favIconUrl":"http://mochi.test:8888/favicon.ico"},{"status":"complete"}]

Not sure where those extra events are coming from.

Comment 1

[Kris Maglione [:kmag]]

I get this failure when I run that test on its own, regardless of the number of content processes. It seems to be because the harness starts up with a tab that hasn't fully loaded.

Lots of other tabs tests blow up when I run with 4 content processes, but this one doesn't seem to be affected.

Comment 2

[Gabor Krizsanits (INACTIVE)]

(In reply to Kris Maglione [:kmag] from comment #1)
> I get this failure when I run that test on its own, regardless of the number
> of content processes. It seems to be because the harness starts up with a
> tab that hasn't fully loaded.

Thanks for looking into this.

> 
> Lots of other tabs tests blow up when I run with 4 content processes, but
> this one doesn't seem to be affected.

Can I ask you how did you run the tests exactly? Because on try I've seen a lot of WE tests failing with process count set to 5 but when I executed only WE tests locally this was the only one failing, so I assumed those other failures were related to some earlier tests in the suit (it happened in other suits as well). (I used custom debug build on OSX for the local tests)

Comment 3

[Kris Maglione [:kmag]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #2)
> > Lots of other tabs tests blow up when I run with 4 content processes, but
> > this one doesn't seem to be affected.
> 
> Can I ask you how did you run the tests exactly? Because on try I've seen a
> lot of WE tests failing with process count set to 5 but when I executed only
> WE tests locally this was the only one failing, so I assumed those other
> failures were related to some earlier tests in the suit (it happened in
> other suits as well). (I used custom debug build on OSX for the local tests)

I set the process count to 4 in all.js and then ran:

  mach mochitest browser/components/extensions/test/browser/

on Linux, with an artifact build. The onUpdated test only failed when I ran:

  mach mochitest browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js

I haven't tried with a debug build, but I wouldn't be surprised if the results
were different, since things would be slower, and some of the issues are
probably timing-dependent.

Comment 4

[Gabor Krizsanits (INACTIVE)]

(In reply to Kris Maglione [:kmag] from comment #3)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #2)
> > > Lots of other tabs tests blow up when I run with 4 content processes, but
> > > this one doesn't seem to be affected.
> > 
> > Can I ask you how did you run the tests exactly? Because on try I've seen a
> > lot of WE tests failing with process count set to 5 but when I executed only
> > WE tests locally this was the only one failing, so I assumed those other
> > failures were related to some earlier tests in the suit (it happened in
> > other suits as well). (I used custom debug build on OSX for the local tests)
> 
> I set the process count to 4 in all.js and then ran:

I set it in firefox.js but that should not make a difference I guess...

> 
>   mach mochitest browser/components/extensions/test/browser/
> 
> on Linux, with an artifact build. The onUpdated test only failed when I ran:
> 
>   mach mochitest
> browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js

With debug build it fails reliably in both cases.

> 
> I haven't tried with a debug build, but I wouldn't be surprised if the
> results
> were different, since things would be slower, and some of the issues are
> probably timing-dependent.

All the other failures I've seen were timeouts and they all come from the forcGC task
that some of (or all?) the tests have as a final task for some reason. But thanks for
your feedback I'm going to try a release build as well and see if that makes a difference
or setting the process count in all.js instead of firefox.js.

Comment 5

[Gabor Krizsanits (INACTIVE)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> your feedback I'm going to try a release build as well and see if that makes
> a difference
> or setting the process count in all.js instead of firefox.js.

Tried both, but got pretty much the same results. Except release build has less timeouts during the GC in the end.

Comment 6

[Kris Maglione [:kmag]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> (In reply to Kris Maglione [:kmag] from comment #3)
> > I haven't tried with a debug build, but I wouldn't be surprised if the
> > results were different, since things would be slower, and some of the
> > issues are probably timing-dependent.
> 
> All the other failures I've seen were timeouts and they all come from the
> forcGC task that some of (or all?) the tests have as a final task for some
> reason.

That's interesting. All of the other failures I see are also timeouts, but
none of them come from the forceGC task. I wonder why additional content
processes would make a difference, though. Do you know if Cu.forceGC affects
content processes? I didn't think it did.

As for why the task is there: for some reason, any windows that we created
that used DOM processes were being kept alive for two full CC cycles,
apparently because of some weird interaction with their preserved wrappers.
That was leading to a lot of random, very long GC pauses, which were causing
unpredictable failures in debug builds. It might not be necessary anymore now
that we have Spidermonkey promises, but I haven't had time to test.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> > your feedback I'm going to try a release build as well and see if that
> > makes a difference or setting the process count in all.js instead of
> > firefox.js.
> 
> Tried both, but got pretty much the same results. Except release build has
> less timeouts during the GC in the end.

The forceGC task is a no-op except in debug builds, so I don't see how it
could affect release builds at all.

Either way, I'll fix the issues with this test. I think we should be safe just
ignoring the irrelevant events from other tabs.

Comment 7

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 8

[Shane Caraveo (:mixedpuppy)]

I was unable to reproduce this, and it's quite old, so moving off p1.  I am inclined to just close it.

Comment 9

[Shane Caraveo (:mixedpuppy)]

test verify wouldn't reproduce, closing.