Closed Bug 1218351 Opened 9 years ago Closed 8 years ago

(e10s) Typed characters sometimes lost right after opening the findbar

Categories

(Toolkit :: Find Toolbar, defect)

Product:
Toolkit
Component:
Find Toolbar
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox46 --- fixed

People

(Reporter: quicksaver, Assigned: quicksaver)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

MozReview Request: Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r?mikedeboer
(deleted), text/x-review-board-request
mikedeboer
: review+
Details
Findbar in Gaia tab (Mulet)
(deleted), image/jpeg
Details
patch to fix fullscreen-api-test
(deleted), patch
smaug
: review+
Details | Diff | Splinter Review 
Spin-off of bug 1198465 but for e10s.

The following steps make this bug easy to reproduce, but the behavior is
inconsistent during normal usage, most noticeable when the browser is busy.

1. Ensure E10S is enabled.

2. Start load of https://html.spec.whatwg.org/

3. Perform the following steps while the page is loading:

   3a. Focus the content area, with a click.

   3b. Initiate "Find in this page" either by clicking Open Menu -> Find or
       Ctrl+F and immediately type one or more characters.

   3c. If the find bar is focused before completion of 3b, then

       3c1. If the page has completed loading, then repeat from 2.

       3c2. Otherwise repeat from 3a.

Expected results:
Find bar contains only and all the characters typed in the latest step 3b.

Actual results:

Find bar is either empty or contains only the last few characters typed; the other characters are lost.
I see that, too.

I also have the same problem when trying to focus the URL bar with Ctrl-L and starting to type.
Characters can be lost there, too.
Hmm, the fix I have in mind for this is specific to the findbar case. Mike, do you think this is better fixed in a more general approach instead (which I don't think I can do), or should I go ahead with the findbar case only?
FWIW I was going to make the brower-content.js's findbar listener [1] to always send the keypress message, and move some of its logic into findbar.xml, so that it can better distinguish when it has been already opened and focused (so that those keypresses should end up in the findbar).

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/content/browser-content.js#629
Flags: needinfo?(mdeboer)
Let's get a fix in for the findbar case first, then we'll see if we can generalize it for the location bar as well.
Flags: needinfo?(mdeboer)
Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r?mikedeboer
Moved some of the routine from content to chrome process. The drawback is that now the content process must wait for a response on every keypress.
Attachment #8681878 - Flags: review?(mdeboer)
While we wait for product (I assume that's what that flag means), I finished the patch I had started already.

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> Let's get a fix in for the findbar case first, then we'll see if we can
> generalize it for the location bar as well.
FWIW by "generalize" I meant this probably happens for every other case where a keycombo changes focus; i.e. search bar, sidebar, popup panels, maybe even modal dialogs? Some of those cases might be extremely difficult to address.
Product triage: Are you needing a decision in terms of priority between Find Bar vs. Location Bar? If so, landing Find Bar first is good and then go ahead and tackle the rest of the use cases in Comment 6. Let's file a new bug to capture the remaining work. Thank you.
Keywords: productwanted
Hey - sorry, we should have been more clear. The productwanted flag was because the e10s team needs to know if this should be a release blocker, because we're not sure.
Flags: needinfo?(vkrishnamoorthy)
Keywords: productwanted
Comment on attachment 8681878 [details]
MozReview Request: Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r?mikedeboer

https://reviewboard.mozilla.org/r/23921/#review21649

I like this approach and it looks like it should fix the issue at hand ;-) Thanks for picking this up - you've got a fan here!

Could you show me another iteration of the patch? I think it should be ready to go at that point.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:650
(Diff revision 1)
> +      let hash = Math.random().toString(16).substr(2, 12);

There are two options for you here to make this work better:
1. keep things as-is, but change the `hash` to a 'global' incrementing integer:
```js
var gSendCharCount = 0;
/*...*/
let seq = ++gSendCharCount;
/*...*/
mm.sendAsyncMessage("Test:SendChar", { char: char, seq: seq });
```
2. rewrite the method using `sendSyncMessage` instead, just like browser-content does. (I'm no e10s guru, but this does make sense to me...)

::: toolkit/content/browser-content.js
(Diff revision 1)
> -  FAYT_LINKS_KEY: "'".charCodeAt(0),

Aha! Removing code-duplication... nice :) (TBH, I never saw this code before...)

::: toolkit/content/tests/browser/browser_findbar.js:182
(Diff revision 1)
> +  let promises = new Set();

There's no need to use a Set in this case, please rewrite this to:

```js
let promises = [
  BrowserTestUtils.sendChar("a", browser),
  BrowserTestUtils.sendChar("b", browser),
  BrowserTestUtils.sendChar("c", browser)
];
```

Just to confirm (haven't checked this myself yet): does this test fail without your changes?

::: toolkit/content/widgets/findbar.xml:750
(Diff revision 1)
> +          // otherwise initial typed characters can be lost.

nit: could you rephrase this as:
"Fast keypresses can stack up when the content process is slow or hangs when in e10s mode. We make sure the finbar isn't 'opened' several times in a row, because else characters typed initially can get lost."

(If you think it's more clear)
Attachment #8681878 - Flags: review?(mdeboer)
https://reviewboard.mozilla.org/r/23921/#review21649

> There are two options for you here to make this work better:
> 1. keep things as-is, but change the `hash` to a 'global' incrementing integer:
> ```js
> var gSendCharCount = 0;
> /*...*/
> let seq = ++gSendCharCount;
> /*...*/
> mm.sendAsyncMessage("Test:SendChar", { char: char, seq: seq });
> ```
> 2. rewrite the method using `sendSyncMessage` instead, just like browser-content does. (I'm no e10s guru, but this does make sense to me...)

> rewrite the method using sendSyncMessage instead

It can only send a sync message from content to chrome; chrome -> content must always be aSync (main process can never wait/lock on content process, so there are no sendSync* methods in chrome messagemanagers). So it'd still have to keep track of the messages sent in some way.

I do like a counter rather than a hash!

> Aha! Removing code-duplication... nice :) (TBH, I never saw this code before...)

> Aha! Removing code-duplication... nice :) (TBH, I never saw this code before...)

Bug 1133981 ;)

> There's no need to use a Set in this case, please rewrite this to:
> 
> ```js
> let promises = [
>   BrowserTestUtils.sendChar("a", browser),
>   BrowserTestUtils.sendChar("b", browser),
>   BrowserTestUtils.sendChar("c", browser)
> ];
> ```
> 
> Just to confirm (haven't checked this myself yet): does this test fail without your changes?

> There's no need to use a Set in this case, please rewrite this to:

Done.

> does this test fail without your changes?

Yep, fails on the last is(): "abc fully entered as find query". Well at least for me on Win8. :)

> nit: could you rephrase this as:
> "Fast keypresses can stack up when the content process is slow or hangs when in e10s mode. We make sure the finbar isn't 'opened' several times in a row, because else characters typed initially can get lost."
> 
> (If you think it's more clear)

> nit: could you rephrase this as:

Perhaps "can get lost" might be too vague even. How about "Fast keypresses can stack up when the content process is slow or hangs when in e10s mode. We make sure the findbar isn't "opened" several times in a row, because then the find query is selected each time, losing characters typed initially.", too specific?
Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r?mikedeboer

Moved some of the routine from content to chrome process. The drawback is that now the content process must wait for a response on every keypress.
Attachment #8683103 - Flags: review?(mdeboer)
Attachment #8681878 - Attachment is obsolete: true
Hmm I'm pretty sure I screwed up the push to the review board. Mike, can you check that everything is alright please? From the docs I understood that the correct procedure for every follow-up patch is to just create a new commit and push it on top of the previous one(s), but looking at how that resulted I'm thinking it's better to re-base with the new changes... (I'm sorry for the bother with this, first time with the review board).
The separate revisions are probably OK for review as reviewboard provides the "squashed diff" view, but hg histedit with fold (or roll perhaps) would be nice before pushing after review.

Usually the process is to replace the previous version, with hg ci --amend for example.  Review board handles that best when it is against the same parent revision (i.e. not rebased), but sometimes it handles rebased patches ok.

Everyone takes some time to find their way around this reviewboard instance :)
Comment on attachment 8681878 [details]
MozReview Request: Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r?mikedeboer

Thanks for the awesome tips, Karl! They were very helpful. :) I'll be sure to create a properly folded version for the next iteration of the patch, or at least for the push (already did it locally).

(Removing this obsolete because it's not really obsolete, yet.)
Attachment #8681878 - Attachment is obsolete: false
https://reviewboard.mozilla.org/r/23919/#review22033

::: toolkit/content/tests/browser/browser_findbar.js:189
(Diff revision 2)
> +        "findbar is not yet focused");

nit: please indent with 2-spaces. This type of alignment is a bit too arbitrary. (same for line 194)

::: toolkit/content/widgets/findbar.xml:288
(Diff revision 2)
> -            // Need to do this in case FAYT
> +            // Need to do this to ensure the correct initial state

nit: missing dot.
Comment on attachment 8683103 [details]
MozReview Request: Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r?mikedeboer

https://reviewboard.mozilla.org/r/24187/#review22031

r=me with nits addressed. Please post a squashed patch to the bug that can be pushed to try and beyond...

::: toolkit/content/widgets/findbar.xml:749
(Diff revision 1)
> -          // in e10s. We make sure the findbar isn't "opened" several times,
> +          // hangs when in e10s mode. We make sure the findbar isn't "opened"

nit: ok, but please quote this with single quotes, not double ones.
Attachment #8683103 - Flags: review?(mdeboer) → review+
Comment on attachment 8681878 [details]
MozReview Request: Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r?mikedeboer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23921/diff/1-2/
Attachment #8681878 - Attachment description: MozReview Request: Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r?mikedeboer → MozReview Request: Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r=mikedeboer
Attachment #8681878 - Flags: review?(mdeboer)
Attachment #8683103 - Attachment is obsolete: true
Comment on attachment 8681878 [details]
MozReview Request: Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r?mikedeboer

https://reviewboard.mozilla.org/r/23921/#review22039

Awesome!
Attachment #8681878 - Flags: review?(mdeboer) → review+
Can you push to try? I don't have permissions for it. ;)

(BTW I noticed it still says r? next to the commit even though it already has the Ship it! below. Is that expected?)
(In reply to Luís Miguel [:quicksaver] from comment #19)
> Can you push to try?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=795789dc766b

> I don't have permissions for it. ;)

I'll vouch if you'd like to apply for level 1 commit access.
https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
https://www.mozilla.org/en-US/about/governance/policies/commit/
 
> (BTW I noticed it still says r? next to the commit even though it already
> has the Ship it! below. Is that expected?)

Not expected.  I don't know what has happened there, but we can ignore that.
(In reply to Karl Tomlinson (ni?:karlt) from comment #20)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=795789dc766b
Shouldn't all builds be tested and not just Linux64? (Were they?)

> I'll vouch if you'd like to apply for level 1 commit access.
I'll definitely do that, still this week hopefully. Thanks!
(In reply to Luís Miguel [:quicksaver] from comment #21)
> Shouldn't all builds be tested and not just Linux64? (Were they?)

Usually only if there is reason to expect different results on different platforms.
Linux machines are cheaper and there are more of them.  (They weren't.)
The bc5 failures seem consistent, though I don't know how they would be related.
Pushed the base revision to try to compare:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a72a6bb8478d
(In reply to Karl Tomlinson (ni?:karlt) from comment #22)
> Usually only if there is reason to expect different results on different
> platforms.

I'd vote to always test at least OS X as well for findbar-related bugs then. It behaves a little differently there: it has its own clipboard, and typeahead is disabled by default. And (while I tested locally) this patch does change the typeahead routine a little. But I defer to you, to decide if it's necessary or not. ;)
(In reply to Luís Miguel [:quicksaver] from comment #24)
> I'd vote to always test at least OS X as well for findbar-related bugs then.
> It behaves a little differently there: it has its own clipboard, and
> typeahead is disabled by default. And (while I tested locally) this patch
> does change the typeahead routine a little.

OK, an OS X run makes sense then, but this patch seems to cause a problem with  browser_use_counters.js, which consistently times out with the patch, but usually doesn't without.

Can you try running this locally to see if you can reproduce, please?
I guess that would be 

./mach mochitest dom/base/test/browser_use_counters.js
Flags: needinfo?(quicksaver)
(In reply to Karl Tomlinson (ni?:karlt) from comment #26)
> OK, an OS X run makes sense then, but this patch seems to cause a problem
> with  browser_use_counters.js, which consistently times out with the patch,
> but usually doesn't without.
> 
> Can you try running this locally to see if you can reproduce, please?
> I guess that would be 
> 
> ./mach mochitest dom/base/test/browser_use_counters.js

Note that I can only run local tests on Windows. I ran the test about a dozen times, both in e10s and non-e10s, no timeouts. From looking at it I don't see how this patch could interfere. :( Could it have had some difficulty accessing the remote test page at the time?
Flags: needinfo?(quicksaver)
I rebased the patch to recent mozilla-central and pushed to try with all platforms.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bf9bd5afea3

(In reply to Luís Miguel [:quicksaver] from comment #27)
>  Could it have had some
> difficulty accessing the remote test page at the time?

I don't know, but if it uses a remote test page, then we can more or less ignore the test as it should be disabled.
Product Triage: Not the best user experience but don't treat this as a blocker.
Keywords: productwanted
Try run in comment 28 looks better.
Keywords: checkin-needed
(In reply to Karl Tomlinson (ni?:karlt) from comment #20)
> I'll vouch if you'd like to apply for level 1 commit access.
> https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
> https://www.mozilla.org/en-US/about/governance/policies/commit/

+1! Please apply for L1, Luís.
something in this push https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=e23d0660a042 caused a perma regression in mulet tests like

https://treeherder.mozilla.org/logviewer.html#?job_id=5714391&repo=fx-team

and
https://treeherder.mozilla.org/logviewer.html#?job_id=5714374&repo=fx-team

to bring the tree back into a stable state i backed out all pushes from this checkin to find out if this clears the mulet bustage
What's the usual proceeding in this situation, checkin again?
Here's a try run including the testsuite failing in comment 34.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bdce770eb35

If that looks good, then add checkin-needed again.
LGTM, bustages and failures seem unrelated. Can you (or someone) look it over and confirm please? I'm still learning all about try runs and might have missed something.
There seem to be a large number of Mulet Linux x64 opt Gij failures including (24) which was the one linked in comment 34.  However, that is a known intermittent, so I've retriggered to see whether the failures are repeatable.
(In reply to Karl Tomlinson (ni?:karlt) from comment #38)
> There seem to be a large number of Mulet Linux x64 opt Gij failures
> including (24) which was the one linked in comment 34.  However, that is a
> known intermittent, so I've retriggered to see whether the failures are
> repeatable.

Those seem related to FxOS homescreen apps, at least in theory this patch really shouldn't interfere with any of that. I'd like to take a look at the tests specifically to make sure, but I can't find them in the mxr... :(
The crashes reported in comment 34 look super unrelated.

I'm going to push to try to confirm, and then let's get this puppy landed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2ace0a033fd
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #41)
> The crashes reported in comment 34 look super unrelated.

Agreed. Let's ship it!
This... this is the weirdest. I'm still seeing the orange on try. With this patch. WTF.

So... this is pretty mysterious. One way of tackling this would be to break up the patch into changes to the individual files, and push the builds to try and test the Mulet thing to see which one is breaking it...

But yeah. This is really odd. :/
My first very own try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdad3e577931

I was going to follow mconley's suggestion (comment 43) already, but I thought it'd be best to have my own base to compare it to. But since it's already there, let's see how true that whole beginner's luck thing really is first.
Turns out it's the changes to browser-content.js:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f46734c4d03b

I assumed this was strictly desktop code. Is this file even used in gaia?

If yes, I wonder if the changes somehow screw up the rocketbar.enterText() calls those failed tests do - examples [1][2] -, it's the only thing I can think of right now anyway.  Tomorrow I'll try to find out exactly which line is causing this issue; maybe one of the removed returns? I'm not familiar enough (or at all) with gaia code to venture a guess right now. Should we bring up someone from gaia dev to this discussion and see if they can shed some light on this?

[1] http://mxr.mozilla.org/gaia/source/apps/system/test/marionette/app_authentication_dialog_test.js#45
[2] http://mxr.mozilla.org/gaia/source/apps/system/test/marionette/browser_chrome_title_test.js#70
Hey asuth - not sure if you'd know this, but you came to mind as a person working on Gaia who might - do we load browser-content.js as a frame script within the special "app" browsers that we create in Gaia?

I, like quicksaver, was under the impression that the frame scripts under toolkit were being used strictly by desktop applications. Is this not true?
Flags: needinfo?(bugmail)
All I'm aware of is:
- in the child:
  - https://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js
  - https://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChild.js
- in the parent:
  - https://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.js

I would be surprised for us to be slurping up stuff from toolkit because our use-cases are so different, but you would want to needinfo :fabrice if those files and related poking around doesn't tell you what you want to know.
Flags: needinfo?(bugmail)
Thanks asuth - fabrice, do you see what we might be running into here?
Flags: needinfo?(fabrice)
I had a thought the other day that I haven't voiced yet because I haven't had a chance to investigate further. Mulet is essentially opening a gaia environment in a tab right? Could those failing tests be simulating keypresses on the "outer browser" that never make it to the "inner gaia tab" (or that make it there but shouldn't)?

I can foresee the FindBar onkeypress listener in browser-content.js intercepting those keypresses if that's the case. In test environment, the preference accessibility.typeaheadfind is false by default, so that listener always no-ops. But with this patch, the listener always sends the sync message to the chrome process. Even though the findbar's message listener should still do nothing in test environment with accessibility.typeaheadfind disabled, could it still interfere somehow?
We don't load browser-content.js on devices or b2g desktop, since it's brought in by toolkit/components/processsingleton/MainProcessSingleton.js that we don't package on b2g. But Mulet inherits the desktop packaging and indeed includes it, so we'll run browser-content.js in gaia's iframes.
On all b2g builds (including mulet) we also load dom/inputmethod/forms.js that deals with text input management. Maybe there's a conflict between these 2 scripts.
Flags: needinfo?(fabrice)
Fabrice, I'm having trouble running a mulet build locally to confirm this. Is the browser findbar usable or useful at all in a gaia tab? If it's not, then the path from here might be pretty straightforward.
Flags: needinfo?(fabrice)
Which kind of issue to you have running mulet? The instructions from https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Developing_Gaia/Different_ways_to_run_Gaia#Using_Gaia_in_Firefox_Mulet worked for me (I used a build from treeherder).

The browser findbar (triggered from ctrl+F on linux) worked as expected for me.
Flags: needinfo?(fabrice)
(In reply to [:fabrice] Fabrice Desré from comment #52)
> Which kind of issue to you have running mulet? The instructions from
> https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Developing_Gaia/
> Different_ways_to_run_Gaia#Using_Gaia_in_Firefox_Mulet worked for me (I used
> a build from treeherder).

That's what I did, I always get a black tab instead of the gaia + devtools environment. Sometimes the browser just crashes too. I'm on Windows though. I'm asking on #fxos about this, but in the meantime...

> The browser findbar (triggered from ctrl+F on linux) worked as expected for
> me.

Hmm... Can/should we set the 'disablefastfind' attribute on gaia's shell selectively for tests only? I don't know how to do this. (I already have a rework of the patch that would make this work as expected.)
Attached image Findbar in Gaia tab (Mulet) (deleted) — 
(In reply to [:fabrice] Fabrice Desré from comment #52)
> The browser findbar (triggered from ctrl+F on linux) worked as expected for
> me.

I finally got it working. I don't mean to doubt your words but I have to say, I don't understand how you can say it works even close to "as expected". First of all, Ctrl+F when the content is focused goes to the devtools' Filter box and not to the findbar, I had to focus the location bar and hit Ctrl+F there to show it. Are you sure you didn't mistake it with the filter box?

Also in a minute of using it:
- it doesn't display fully (screenshot);
- it does not update correctly its state (notice in the screenshot it's always "Find not found" even when it should be found;
- it is barely able to find any text, so far I could only find (a few things) in the pinned pages screen and very few other specific places where it's obvious there are text nodes;
- no matches counter;
- most times I can't remove old highlights;
- mid-way of writing this, it stopped working altogether.

Personally, I would disable the findbar in the gaia tab in this bug to land it, and let others more familiar with gaia dev fix it (both usage and tests) in other bug(s) and re-enable it then. :/
Flags: needinfo?(fabrice)
(In reply to Luís Miguel [:quicksaver] from comment #54)

> I finally got it working. I don't mean to doubt your words but I have to
> say, I don't understand how you can say it works even close to "as
> expected". First of all, Ctrl+F when the content is focused goes to the
> devtools' Filter box and not to the findbar, I had to focus the location bar
> and hit Ctrl+F there to show it. Are you sure you didn't mistake it with the
> filter box?

I have to focus the gaia content, yes. But that's expected

> Also in a minute of using it:
> - it doesn't display fully (screenshot);

It does, but the background is wrong on the right side. This Looks like an issue with the responsive mode feature, as I can repro on a regular firefox with devtools & responsive mode.

> - it does not update correctly its state (notice in the screenshot it's
> always "Find not found" even when it should be found;
> - it is barely able to find any text, so far I could only find (a few
> things) in the pinned pages screen and very few other specific places where
> it's obvious there are text nodes;
> - no matches counter;
> - most times I can't remove old highlights;
> - mid-way of writing this, it stopped working altogether.

I can repro these issues intermittently after a bit more testing. I guess I was just lucky the first time.

> Personally, I would disable the findbar in the gaia tab in this bug to land
> it, and let others more familiar with gaia dev fix it (both usage and tests)
> in other bug(s) and re-enable it then. :/

I don't have any objection disabling that for gaia, but I would not expect gaia devs to fix that either.
Flags: needinfo?(fabrice)
Comment on attachment 8681878 [details]
MozReview Request: Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r?mikedeboer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23921/diff/2-3/
Attachment #8681878 - Attachment description: MozReview Request: Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r=mikedeboer → MozReview Request: Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r?mikedeboer
First, had to rebase because of bug 1190382.

Gaia's shell.ini is not what is loaded during gaia integration tests after all [1].

I can't figure out what actually is loaded in the tab at that point because:
i) taskcluster tasks for those tests (in try runs) don't show console output;
ii) of course I can't run these tests locally because that would just be too easy...
iii) Does anyone know what is actually loaded in the tab during those tests? Or if you can run the patch from [1] locally it will tell you in the console output.

I noticed the previous iteration of the patch initialized the findbar on every keypress. That's of course bad in itself, hence the new patch. I split part of the logic of shouldFastFind() into a canFastFind(), which lets us bail out early before the sync message is sent in documents where the findbar can't even be used.

It could also have been that useless findbar initialization that was interfering with the tests. It's not [2]. It must be the actual sync message somehow busting those tests, although I have no clue why. (WIP try run that always returns before the message is sent but after canAndShouldFastFind is run. [3])

Does anyone have any other suggestions to bail out of the content handler before the message is sent during these tests? Figuring out what's actually loaded in the tab would be awesome!

You guys work on that while I make a little trip to the village, and fetch one of my uncle's goats to sacrifice to the mighty god Ra...

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=186a670af6b1
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e12aabc37769
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=33456ca21656
(In reply to Luís Miguel [:quicksaver] from comment #57)
> Gaia's shell.ini is not what is loaded during gaia integration tests after
> all [1].
I of course meant shell.html

> I can't figure out what actually is loaded in the tab at that point because:
> i) taskcluster tasks for those tests (in try runs) don't show console output;
> ii) of course I can't run these tests locally because that would just be too
> easy...
> iii) Does anyone know what is actually loaded in the tab during those tests?
> Or if you can run the patch from [1] locally it will tell you in the console
> output.
> 
> [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=186a670af6b1

Andrew, Fabrice, would either of you happen to know this / can find this out?
Flags: needinfo?(fabrice)
Flags: needinfo?(bugmail)
Gijs, you originally wrote this code in bug 1133981, did you run into anything like this at the time? Would you happen to have any idea of what might be going on?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Luís Miguel [:quicksaver] from comment #59)
> Gijs, you originally wrote this code in bug 1133981, did you run into
> anything like this at the time? Would you happen to have any idea of what
> might be going on?

Not offhand. I know literally nothing about mulet and issues you would have there.
Flags: needinfo?(gijskruitbosch+bugs)
Ping. Anyone? I really won't be able to do anything here unless someone helps me with these issues. It's a shame we can't finish this because of incompatibilities with completely unrelated mulet tests, of all things... :(
(In reply to Luís Miguel [:quicksaver] from comment #61)
> Ping. Anyone? I really won't be able to do anything here unless someone
> helps me with these issues. It's a shame we can't finish this because of
> incompatibilities with completely unrelated mulet tests, of all things... :(

I can try to look next week, but someone who actually knows Mulet is more likely to know what's going on...
Flags: needinfo?(gijskruitbosch+bugs)
Per discussion on IRC, maybe gerard-majax and/or Alexandre know what is going on here? For context, we're seeing failing trypushes in mulet when trying to modify findbar to fix e10s issues on desktop:

(In reply to Luís Miguel [:quicksaver] from comment #57)
> First, had to rebase because of bug 1190382.
> 
> Gaia's [shell.html] is not what is loaded during gaia integration tests after
> all [1].
> 
> I can't figure out what actually is loaded in the tab at that point because:
> i) taskcluster tasks for those tests (in try runs) don't show console output;
> ii) of course I can't run these tests locally because that would just be too
> easy...
> iii) Does anyone know what is actually loaded in the tab during those tests?
> Or if you can run the patch from [1] locally it will tell you in the console
> output.
> 
> I noticed the previous iteration of the patch initialized the findbar on
> every keypress. That's of course bad in itself, hence the new patch. I split
> part of the logic of shouldFastFind() into a canFastFind(), which lets us
> bail out early before the sync message is sent in documents where the
> findbar can't even be used.
> 
> It could also have been that useless findbar initialization that was
> interfering with the tests. It's not [2]. It must be the actual sync message
> somehow busting those tests, although I have no clue why. (WIP try run that
> always returns before the message is sent but after canAndShouldFastFind is
> run. [3])

> [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=186a670af6b1
> [2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e12aabc37769
> [3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=33456ca21656
Flags: needinfo?(poirot.alex)
Flags: needinfo?(lissyx+mozillians)
Flags: needinfo?(gijskruitbosch+bugs)
I'm off today, but I'll try to take a look early next week if noone did by then.
I imagine, as fabrice suggested, this may not be fastfind itself breaking tests,
but may be more a code conflict somewhere.
Your tentative to put disablefastfind seems logic and I think it worked, but isn't enough to actually fix the yet unknown issue.
It's hard for me to even get to the point of understanding what I am being asked for there.
Flags: needinfo?(lissyx+mozillians)
Flags: needinfo?(bugmail)
(In reply to Alexandre Poirot [:ochameau] from comment #64)
> Your tentative to put disablefastfind seems logic and I think it worked, but
> isn't enough to actually fix the yet unknown issue.

I'm not sure I agree that it worked, it didn't seem to work at all. The difference between try runs 1 and 3 in comment 57 is that try 3 forces a bail-out at the point where it would have bailed out anyway if disablefastfind was true. Only the following could in theory be happening there:

a) The problem comes before that check, which seems unlikely (impossible?) otherwise try 3 would have also failed.
b) shell.html is not the document loaded in the tab during the tests. This is the most likely I think.
c) Checking for the disablefastfind attribute fails for that specific document, for whatever reason. How can we even check for this?
d) And of course I could have messed up somewhere, entirely possible as well. But I've already looked at it from every possible angle...
I'm not able to reproduce the Gaia test failure. But I had to rebase your patch, I'm wondering if I did it correctly. Could you rebase it?

Note that browser.xul isn't involved when running these tests, even on Mulet.
We load shell.html as top level window.

If you want to run a gaia test on mulet, you should do something like that:
RUNTIME=.../obj-mulet/dist/bin/firefox-bin TEST_FILES=apps/system/test/marionette/app_window_mananger_pinned_sites_test.js make test-integration-test
But before that, you have to do manyyy things. first have nodejs4, clang installed.
Then run "make test-integration", it will setup many things but try to run all tests, you can CTRL+C once the first test runs and then run your test.
Flags: needinfo?(poirot.alex)
Comment on attachment 8681878 [details]
MozReview Request: Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r?mikedeboer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23921/diff/3-4/
The findbar also does not work properly in gaia tabs. More than the tests, its usage needs to be fixed if/when re-enabling it there later.

Review commit: https://reviewboard.mozilla.org/r/30389/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30389/
Attachment #8706569 - Flags: review?(mdeboer)
(In reply to Alexandre Poirot [:ochameau] from comment #67)
> I'm not able to reproduce the Gaia test failure. But I had to rebase your
> patch, I'm wondering if I did it correctly. Could you rebase it?
Rebased and uploaded to MozReview. And pushed to try again to make sure those tests are still failing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39f25b1471b9

> Note that browser.xul isn't involved when running these tests, even on Mulet.
> We load shell.html as top level window.
Just to confirm, you mean shell.html is loaded as top level - in a tab in a "normal" Firefox window (browser.xul), right? Otherwise, there would be no reason for why these changes would affect mulet tests at all.

> If you want to run a gaia test on mulet, you should do something like that:
> RUNTIME=.../obj-mulet/dist/bin/firefox-bin
> TEST_FILES=apps/system/test/marionette/app_window_mananger_pinned_sites_test.
> js make test-integration-test
> But before that, you have to do manyyy things. first have nodejs4, clang
> installed.
> Then run "make test-integration", it will setup many things but try to run
> all tests, you can CTRL+C once the first test runs and then run your test.
That's what I did at the time (in addition to a few variations when running and setting up everything), it never seemed to run any tests even though no (obvious) errors were shown in the terminal. I'll try again from scratch tomorrow.
Flags: needinfo?(poirot.alex)
(In reply to Luís Miguel [:quicksaver] from comment #70)
> > Note that browser.xul isn't involved when running these tests, even on Mulet.
> > We load shell.html as top level window.
> Just to confirm, you mean shell.html is loaded as top level - in a tab in a
> "normal" Firefox window (browser.xul), right? Otherwise, there would be no
> reason for why these changes would affect mulet tests at all.

There is just shell.html being loaded (and various iframes/documents in it related to gaia).
No browser.xul at all!
But see comment 50 of fabrice. We still execute browser-content.js, even without browser.xul.
MainProcessSingleton.js will execute even without loading browser.xul.
Having said that, I have absolutely now idea why your change in browser-content.js would break anything...


> 
> > If you want to run a gaia test on mulet, you should do something like that:
> > RUNTIME=.../obj-mulet/dist/bin/firefox-bin
> > TEST_FILES=apps/system/test/marionette/app_window_mananger_pinned_sites_test.
> > js make test-integration-test
> > But before that, you have to do manyyy things. first have nodejs4, clang
> > installed.
> > Then run "make test-integration", it will setup many things but try to run
> > all tests, you can CTRL+C once the first test runs and then run your test.
> That's what I did at the time (in addition to a few variations when running
> and setting up everything), it never seemed to run any tests even though no
> (obvious) errors were shown in the terminal. I'll try again from scratch
> tomorrow.

Running gaia tests is often a quest by itself... I'll run them again against your rebased patch tomorrow.
Flags: needinfo?(poirot.alex)
Still can't reproduce locally, nor does it reproduce on try.
I'm not sure your second patch is necessary? It doesn't reproduce either if I remove the shell.html modification.
Flags: needinfo?(fabrice)
Comment on attachment 8706569 [details]
MozReview Request: Bug 1218351 - Disable findbar in gaia tabs so its tests can cope. r?mikedeboer

https://reviewboard.mozilla.org/r/30389/#review27233

LGTM!
Attachment #8706569 - Flags: review?(mdeboer) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #72)
> Still can't reproduce locally, nor does it reproduce on try.
> I'm not sure your second patch is necessary? It doesn't reproduce either if
> I remove the shell.html modification.

O.o Of course that the tests would succeed now that the bug has gathered interest by more people... https://treeherder.mozilla.org/#/jobs?repo=try&revision=6631ac13182b I can't even...

Here's a more complete try with the rebased patch without the change to gaia's shell.ini:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8a6c353f3fc
Mike, if the above's green what's the verdict? Is it absolutely necessary to find out which changeset fixed those previous test failures (which I'm sure I can't do myself), or should I just update the patch in MozReview without the changes to shell.ini, check it in, and consider everything was fixed by the power of love from everyone that gathered here?

BTW, I'm almost sure you haven't reviewed the main patch since revision 2. Was that because it wasn't really worth doing it since we couldn't get past those gaia tests or did you just miss it in-between everything? ( Was that my fault? I'm not sure how to reset the r+ flag here since in MozReview it's r? )
Flags: needinfo?(mdeboer)
(In reply to Luís Miguel [:quicksaver] from comment #75)
> Mike, if the above's green what's the verdict? Is it absolutely necessary to
> find out which changeset fixed those previous test failures (which I'm sure
> I can't do myself), or should I just update the patch in MozReview without
> the changes to shell.ini, check it in, and consider everything was fixed by
> the power of love from everyone that gathered here?

More things should be fixed by the power of love!

> BTW, I'm almost sure you haven't reviewed the main patch since revision 2.
> Was that because it wasn't really worth doing it since we couldn't get past
> those gaia tests or did you just miss it in-between everything? ( Was that
> my fault? I'm not sure how to reset the r+ flag here since in MozReview it's
> r? )

I didn't, indeed. It'd be awesome if you could upload an interdiff of the changes you made between the patch I reviewed and the one you're asking me to review now... can you do that? If so, please upload them as a regular patch to the bug.
Flags: needinfo?(mdeboer) → needinfo?(quicksaver)
MozReview has working interdiffs, thankfully! You gave an r+ on revision 2, and we're up to revision 4 on that patch. Here's the interdiff:

https://reviewboard.mozilla.org/r/23921/diff/2-4/
Flags: needinfo?(quicksaver)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #77)
> MozReview has working interdiffs, thankfully! You gave an r+ on revision 2,
> and we're up to revision 4 on that patch. Here's the interdiff:
> 
> https://reviewboard.mozilla.org/r/23921/diff/2-4/

Gosh, that's quite awesome, really! Thanks!
FYI, I'm already investigating all that orange in that try. I must have missed something.
Comment on attachment 8681878 [details]
MozReview Request: Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r?mikedeboer

https://reviewboard.mozilla.org/r/23921/#review27525

::: toolkit/content/browser-content.js:616
(Diff revisions 2 - 4)
> +    let {BrowserUtils} = Cu.import("resource://gre/modules/BrowserUtils.jsm", {});

nit: can you fix the indentation here?

::: toolkit/content/browser-content.js:623
(Diff revisions 2 - 4)
> +        focusedWindow);

nit: can you keep this on the same line, like it was before?

::: toolkit/content/browser-content.js:647
(Diff revisions 2 - 4)
> +          !(k in content.KeyboardEvent)) {

Why is this necessary? If compelling, can you add a comment to describe why?

::: browser/base/content/tabbrowser.xml:4155
(Diff revision 4)
> +            case "live-resize-start": {

Please remove the braces for the case-blocks.

::: browser/base/content/tabbrowser.xml:4178
(Diff revision 4)
> +                Services.prefs.getBoolPref("accessibility.typeaheadfind");

nit: 80-char limit is a bit harsh, so we're accepting 80-120 char limit. It may exceed the 80 character limit IF it improves legibility.

::: browser/base/content/tabbrowser.xml:4258
(Diff revision 4)
> +          this._findAsYouType =

nit: same with character limit as above. Please put it one a single line if possible.
Attachment #8681878 - Flags: review+
(In reply to Mike de Boer [:mikedeboer] from comment #80)
> ::: toolkit/content/browser-content.js:647
> (Diff revisions 2 - 4)
> > +          !(k in content.KeyboardEvent)) {
> 
> Why is this necessary? If compelling, can you add a comment to describe why?

This is just moved from bug 1190382. It avoids sending all the funky VK_FOO key constants with every message.
https://reviewboard.mozilla.org/r/23921/#review27525

> nit: can you fix the indentation here?

If I'm looking at the same thing you are, the indentation is correct, four spaces total. The extra couple of pixels I think come from the "Moved from line..." blue marker.

> Why is this necessary? If compelling, can you add a comment to describe why?

As Gijs mentioned in comment 81, that's not from this patch. How peculiar of MozReview to show it in this diff as well...

> Please remove the braces for the case-blocks.

Removing the braces causes a "TypeError: redeclaration of let browser", so can't. :) It needs to stay enclosed in that block, or be declared outside of the switch, which I find unnecesarry since it's not really needed all the time; i.e. when "nsPref:changed".
(In reply to Luís Miguel [:quicksaver] from comment #82)
> If I'm looking at the same thing you are, the indentation is correct, four
> spaces total. The extra couple of pixels I think come from the "Moved from
> line..." blue marker.

You're right, it's the blue marker.

> 
> > Why is this necessary? If compelling, can you add a comment to describe why?
> 
> As Gijs mentioned in comment 81, that's not from this patch. How peculiar of
> MozReview to show it in this diff as well...

Aha! Of course... Well, nice reminder why comments are good to have and request.

> Removing the braces causes a "TypeError: redeclaration of let browser", so
> can't. :) It needs to stay enclosed in that block, or be declared outside of
> the switch, which I find unnecesarry since it's not really needed all the
> time; i.e. when "nsPref:changed".

I'd prefer declaring `let browser` before the switch.
Comment on attachment 8681878 [details]
MozReview Request: Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r?mikedeboer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23921/diff/4-5/
Attachment #8681878 - Flags: review?(mdeboer)
Attachment #8706569 - Attachment is obsolete: true
Interdiff with changes to shell.ini removed: https://reviewboard.mozilla.org/r/23919/diff/5-6/

Addressed comments. Fixed all the orangeness and redness (hopefully, looks good locally): https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ae21915efa5
Comment on attachment 8681878 [details]
MozReview Request: Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r?mikedeboer

https://reviewboard.mozilla.org/r/23921/#review27591

With a green try run, let's ship this to the world! Thanks Luís!
Attachment #8681878 - Flags: review?(mdeboer) → review+
:xidorn, this patch breaks the new browser_fullscrean-api-keys.js (bug 1191597). Apparently the sendSyncMessage() call in browser-content.js's FindBar._onKeypress causes the keydown and keypress events to be suppressed when synthesizing F11 (which works fine when tested manually btw). The test fails when receiving keyup when it expects keydown: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ae21915efa5

Two possible solutions:
- make that test page local and disable the findbar in it;
- expect F11 to be suppressed like Esc.

I'm wary of making either change because I don't know what exactly either implies for that test, and how (in)accurate that would make it. I'm not even sure those events should be suppressed like that, it could be a bug in the event cascade itself.
Flags: needinfo?(quanxunzhen)
Comment on attachment 8681878 [details]
MozReview Request: Bug 1218351 - (e10s) Don't lose initial typed characters when opening the findbar; r?mikedeboer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23921/diff/5-6/
(In reply to Luís Miguel [:quicksaver] from comment #87)
> Apparently the sendSyncMessage() call in browser-content.js's
> FindBar._onKeypress causes the keydown and keypress events to be suppressed
> when synthesizing F11 (which works fine when tested manually btw).
Well apparently those events aren't suppressed at all. On the contrary, they are fired even before expected... Fascinating.

Even more peculiar, I can't seem to trigger a new try with the updated code from within the reviewboard... Here's a manual push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59e474f72775

Mike, do you agree with this approach or would you rather see it fixed in another way? Xidorn, do you agree with these changes to browser_fullscreen-api-keys.js?
I'm confused. What really happens here?

When the chrome code calls synthesizeKey, the frame script receives the key events and send three messages. The receiveExpectedKeyEvents function then get the messages. What breaks this flow here? Why is the messages lost?

If we need to register the message handler beforehand, I'd prefer a different solution. Let me work on that patch.
Flags: needinfo?(quanxunzhen)
Attachment #8707933 - Flags: review?(quanxunzhen)
:quicksaver, could you check whether this patch fixes the issue?
Flags: needinfo?(quicksaver)
Attachment #8708161 - Flags: review?(bugs)
Attachment #8707933 - Attachment is obsolete: true
Attachment #8707933 - Flags: review?(mdeboer)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #92)
> I'm confused. What really happens here?
> 
> When the chrome code calls synthesizeKey, the frame script receives the key
> events and send three messages. The receiveExpectedKeyEvents function then
> get the messages. What breaks this flow here? Why is the messages lost?

The keydown and keypress events are sent synchronously to the chrome process if something in the event cascade in the content process is done synchronously with the chrome process as well. (This is how I'm understanding it at least, I don't know the specifics of how/why this happens, so that explanation may not be 100% correct.)

This test patch should help you visualize it better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5279592cfdd1

BTW I noticed this but forgot to mention it before, captureUnexpectedKeyEvent() won't report the received event correctly because its argument is a message object, not the type string directly.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #93)
> Created attachment 8708161 [details] [diff] [review]
> patch to fix fullscreen-api-test
> 
> :quicksaver, could you check whether this patch fixes the issue?

It does fix it. I've removed my changes to that test from the reviewboard. Here's at try run with both applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ebda6713df8
Flags: needinfo?(quicksaver)
Comment on attachment 8708161 [details] [diff] [review]
patch to fix fullscreen-api-test

rs+.
Attachment #8708161 - Flags: review?(bugs) → review+
Code is reviewed, try push is green. Let's see if this sticks this time. *prays in Javascript*

Checkin is for both patches listed: the one in the reviewboard and the one attached here.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/921f49427e80
https://hg.mozilla.org/mozilla-central/rev/8162dea7a7c7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
\o/ Congrats Luís and thanks for pulling through here ;-) Also thanks to everyone involved for helping out!
Flags: needinfo?(vkrishnamoorthy)
Yeah, big props to Quicksaver on sticking that out. That was a total slog.
I just want to mention here for posterity sake that this patch broke my own add-on... Awesome. My thanks to everyone who lent a hand here. +1 to the power of love.
Depends on: 1248683
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: