1294383 - [e10s-multi] browser_bug562649.js fails with multiple content processes

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Gabor Krizsanits (INACTIVE)]

browser/base/content/test/urlbar/browser_bug562649.js | userTypedValue matches test URI after switching tabs - Got null, expected data:text/plain,bug562649

seems like a timing issue, maybe a couple of missing yields or something...

Comment 1

[Gabor Krizsanits (INACTIVE)]

One thing that's an issue here is a simple timeout like in bug 1294388. Once I fix that though the test still fails. The problem is that this test is based on an assumption that if one calls openURI and then opens then closes a tab, that happens sooner than the _start_ of the loading operation, where we reset the userTypedValue. That used to be the case, but there is a race between the two (permitUnload of tab closing and starting the load), and with the extra delay I talk about in bug 1294388 we get unlucky and the test fails.

This test used to test a very specific bug. But it seems like Gijs removed that code that used to prevent that bug, so we should not worry about that problem any more. I'm pretty sure that it's now a meaningless test. In fact I'm not even sure that LOAD_FLAGS_FROM_EXTERNAL does anything at all. Anyway, Gijs, after your patch in bug 1241085 it seems to me that removing:

  if (!(flags & browser.webNavigation.LOAD_FLAGS_FROM_EXTERNAL)) {
    browser.userTypedClear++;

Made this test meaningless. Do you know any reason why I should not just remove it? I see no way to make this test pass with e10s-multi and I would rewrite it to test the original problem differently, but I don't think that the original problem is really an issue any more.

Comment 2

[:Gijs (he/him)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #1)
> I see no way to make this test pass with e10s-multi and I would
> rewrite it to test the original problem differently, but I don't think that
> the original problem is really an issue any more.

Why not?

Comment 3

[Gabor Krizsanits (INACTIVE)]

(In reply to :Gijs Kruitbosch from comment #2)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #1)
> > I see no way to make this test pass with e10s-multi and I would
> > rewrite it to test the original problem differently, but I don't think that
> > the original problem is really an issue any more.
> 
> Why not?

I hoped you would know because you removed that line that fixed that problem (the problem I'm referring to: bug 562649 comment 2). If it were an issue your patch would have broken this test.

Comment 4

[:Gijs (he/him)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> (In reply to :Gijs Kruitbosch from comment #2)
> > (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #1)
> > > I see no way to make this test pass with e10s-multi and I would
> > > rewrite it to test the original problem differently, but I don't think that
> > > the original problem is really an issue any more.
> > 
> > Why not?
> 
> I hoped you would know because you removed that line that fixed that problem
> (the problem I'm referring to: bug 562649 comment 2). If it were an issue
> your patch would have broken this test.

Well, no, not if the test is racy or wrong as you described. It turns out writing a URL bar clearing/updating change is hard. I did my best to verify things and correct/rewrite tests where necessary, but it's possible I missed something.

The real question is whether the behaviour described in bug 562649 is currently correct or not, and why (not). The quoted bit of your statement suggests that the actual *problem* is not an issue anymore, which is an interesting assertion. Am I just reading too much into it?

Comment 5

[Gabor Krizsanits (INACTIVE)]

(In reply to :Gijs Kruitbosch from comment #4)
> Well, no, not if the test is racy or wrong as you described. It turns out
> writing a URL bar clearing/updating change is hard.

After debugging this for a few days, I could not agree with you any more.

> 
> The real question is whether the behaviour described in bug 562649 is
> currently correct or not, and why (not). The quoted bit of your statement
> suggests that the actual *problem* is not an issue anymore, which is an
> interesting assertion. Am I just reading too much into it?

Oh, I think my English was bad again. If we encountered the same bug as
562649 was originally about we should totally care. I just don't think this test reliably
testing that any more. But it does not matter, with the latest version that fixes bug 1294388
seems to be fixing this one as well without any side effects, so I'll just go with this.

Comment 6

[Gabor Krizsanits (INACTIVE)]

Attachment: increasing permitUnload timeout for the failing test

Comment 7

[Gabor Krizsanits (INACTIVE)]

This should be fixed by bug 1211637.