Closed Bug 1303008 Opened 3 years ago Closed 3 years ago

Find-in-page in Fennec doesn't update properly

Categories

(Firefox for Android :: General, defect)

50 Branch
All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 52
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 + verified
firefox51 --- verified
firefox52 --- verified

People

(Reporter: kats, Assigned: mikedeboer)

References

Details

(Keywords: regression)

Attachments

(1 file)

[Tracking Requested - why for this release]: Embarassing regression in usability of the find-in-page

STR (aurora 50 or nightly 51):
1. Load a page, e.g. https://bug1245083.bmoattachments.org/attachment.cgi?id=8791492
2. Bring up the find-in-page and type "v=5" (without the quotes, one character at a time)

ER: Match counter shows "1/4". AR: match counter shows "1/27"

3. Press the down arrow once. At this point the match counter should update to 1/4. Press the down arrow again to advance to 2/4.
4. Now press the UP arrow.

ER: match counter goes back to 1/4. AR: match counter shows 3/4.

This is not only wrong but makes the match counter totally unreliable and so it's impossible to tell which match you're actually on. Use cases where you are interested in the order of the matches in the document are basically broken.
I used mozregression to bisect this down. The culprit is bug 1281421, with an unambiguous range [1]. Mike, can you please take a look? This regression will be on beta after the merge, so the sooner the better. Thanks!

[1] https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5e41e5886c0f7e92c8adb338d8c939c0f25807ea&tochange=f962e7e8fb1099c29e3ff816ff0eb0a2c8fff5df
Blocks: 1281421
Flags: needinfo?(mdeboer)
Of course! I think this can be fixed by uplifting a patch I wrote earlier but forgot to request approval for.
Flags: needinfo?(mdeboer)
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 51.3 - Sep 19
Points: --- → 2
Flags: qe-verify+
(In reply to Mike de Boer [:mikedeboer] from comment #2)
> Of course! I think this can be fixed by uplifting a patch I wrote earlier
> but forgot to request approval for.

Thanks for the quick response! Note that the bug is still present in the latest nightly, so if the patch you're referring to already landed then it didn't fix the problem.
My locally built Fennec on the OSX android emulator is showing blank pages for everything I enter in the address bar, but when I switch tabs I do see that thumbnails are there.
What's going on?
Flags: needinfo?(bugmail)
As discussed on IRC I'm not sure how to resolve this. I suggested trying on a real device if one is available, or on a Linux machine. Another option would be to try flipping the HW/GPU emulation settings in the emulator but I'm not sure how to persist those because ./mach run provides its own config when running the emulator.

jchen might know what's going on here.
Flags: needinfo?(bugmail) → needinfo?(nchen)
What's in the `adb logcat` output? I'm pretty sure Fennec wouldn't run at all if HW GPU is not turned on, so this could be a real bug.
Flags: needinfo?(nchen) → needinfo?(mdeboer)
Tracked since it's a new issue in Fx50.
(In reply to Jim Chen [:jchen] [:darchons] from comment #6)
> What's in the `adb logcat` output? I'm pretty sure Fennec wouldn't run at
> all if HW GPU is not turned on, so this could be a real bug.

Sorry for the late reply!

I've put the output in this pastebin: https://pastebin.mozilla.org/8912949
Especially the error 'ERROR: Valid GLSL but not GLSL ES' stands out to me.
Flags: needinfo?(mdeboer) → needinfo?(nchen)
Hm, I'm not familiar with GLSL. Maybe the emulator's GLES version is too low? Or did we up our GLES version requirement?
Flags: needinfo?(nchen) → needinfo?(bugmail)
I'm not really sure. I just tried doing a Fennec build on OS X to see if I could reproduce the issues you're having (or maybe find out what my configuration is so that I can get you to try it) but I ran into a different problem, for which I filed bug 1305524. Once that is resolved I'll try again.
Flags: needinfo?(bugmail)
See Also: → 1305524
Ok, I was able to get fennec up and running in the emulator. When I run "ps aux | grep emulator" I see that the emulator is running with the following command line:

/Users/kats/.mozbuild/android-sdk-macosx/tools/emulator64-arm -avd mozemulator-4.3 -port 5554 -show-kernel -debug init,console,gles,memcheck,adbserver,adbclient,adb,avd_config,socket

Can you confirm you have the same thing?

Also during my emulator startup I see this go by in the logcat:

D/libEGL  (  333): loaded /system/lib/egl/libEGL_emulation.so
D/        (  333): HostConnection::get() New Host Connection established 0x2a1bcba0, tid 333
D/libEGL  (  333): loaded /system/lib/egl/libGLESv1_CM_emulation.so
D/libEGL  (  333): loaded /system/lib/egl/libGLESv2_emulation.so
W/EGL_emulation(  333): eglSurfaceAttrib not implemented
D/OpenGLRenderer(  333): Enabling debug mode 0

Your logcat has similar things. And then finally when Gecko is loading I see this:

I/Gecko   (  573): Attempting load of libEGL.so
D/        (  573): HostConnection::get() New Host Connection established 0x2a375ac8, tid 589
<snip>
W/GeckoScreenOrientation(  573): screenOrientationFromString: unknown orientation string:
D/GeckoLayerClient(  573): Screen-size changed to (720,1280)
D/GeckoLayerClient(  573): Window-size changed to (720,1134)
V/GeckoDynamicToolbarAnimator(  573): Requested immediate toolbar animation to translation 0.0
V/GeckoDynamicToolbarAnimator(  573): Changing animation to immediate jump
I/GeckoDisplayPort(  573): Set strategy VelocityBiasStrategy mult=2.0, threshold=10.240001, reverse=0.2, dangerBaseX=1.0, dangerBaseY=1.0, dangerIncrX=0.0, dangerIncrY=0.0
D/        (  573): HostConnection::get() New Host Connection established 0x2a26d050, tid 632
I/GeckoConsole(  573): OpenGL compositor Initialized Succesfully.
I/GeckoConsole(  573): Version: OpenGL ES 2.0 (2.1 INTEL-10.14.73)
I/GeckoConsole(  573): Vendor: Google (Intel Inc.)
I/GeckoConsole(  573): Renderer: Android Emulator OpenGL ES Translator (Intel(R) HD Graphics 6000)
I/GeckoConsole(  573): FBO Texture Target: TEXTURE_2D

Whereas in your logcat you get the shader compilation failure at this point. The shader in question lives at [1] and from the error message in your log ("Valid GLSL but not GLSL ES") it sounds like maybe GL_ES isn't defined?

[1] http://searchfox.org/mozilla-central/rev/767e1e9b118269f957ca1817138472146278a29e/gfx/layers/opengl/OGLShaderProgram.cpp#174
Jeff, do you have any thoughts on what could be going wrong here? In a nutshell, Mike is trying to run Fennec in the emulator on OS X but he's hitting a shader compilation failure. His output is at https://pastebin.mozilla.org/8912949 and the error message is "ERROR: Valid GLSL but not GLSL ES". I was able to run Fennec successfully in the emulator on my OS X machine so it might be hardware specific.
Flags: needinfo?(jmuizelaar)
I have no guesses really. It could be worth trying to manually reduce the shaders to see what it's complaining about. That might give more of a hint as to what's going on.
Flags: needinfo?(jmuizelaar)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> /Users/kats/.mozbuild/android-sdk-macosx/tools/emulator64-arm -avd
> mozemulator-4.3 -port 5554 -show-kernel -debug
> init,console,gles,memcheck,adbserver,adbclient,adb,avd_config,socket
> 
> Can you confirm you have the same thing?

I see the exact same thing.

> Also during my emulator startup I see this go by in the logcat:
> 
> D/libEGL  (  333): loaded /system/lib/egl/libEGL_emulation.so
> D/        (  333): HostConnection::get() New Host Connection established
> 0x2a1bcba0, tid 333
> D/libEGL  (  333): loaded /system/lib/egl/libGLESv1_CM_emulation.so
> D/libEGL  (  333): loaded /system/lib/egl/libGLESv2_emulation.so
> W/EGL_emulation(  333): eglSurfaceAttrib not implemented
> D/OpenGLRenderer(  333): Enabling debug mode 0

Indeed, exact same for me.

I saw that your .mozconfig has different entries than I do. Maybe I can try that?
Flags: needinfo?(bugmail)
After running mach bootstrap I updated my mozconfig to use the SDK that it recommended, so now my mozconfig looks different than it used to. I just pushed the latest version to https://github.com/staktrace/moz-scripts/blob/master/mozconfig.Darwin-android-ndebug - if you see any differences compared to yours that you think are significant feel free to try that.

Also given how long this is taking I would suggest just buying an Android phone and expensing it. Or if you're in an office, ask around - I'm sure somebody has a spare one you can borrow.
Flags: needinfo?(bugmail)
Heh, very true. Will do!
I'm seeing the same behaviour as described in Comment 4. Logcat: https://pastebin.mozilla.org/8913967 -> same "ERROR: Valid GLSL but not GLSL ES" message present.

I can get around this by changing Android Emulator settings, specifically setting "Graphics" setting to "Software - GLES 2.0". Default is "Automatic", and "Hardware - GLES 2.0" is another option in there. Both "hardware" and "automatic" produce the above behaviour. With "Software" everything appears to work, but rather slowly.

Running this on Fedora 24, on a thinkpad w541. I don't have this problem running on Fedora 24 on an thinkpad X1.
I turned on GL tracing to logcat and get the following output:

https://pastebin.mozilla.org/8914062

Grisha, how do I get to the emulator settings? I don't have Android Studio installed, just mozilla-central cloned & built. When I start the emulator with ./mach run I don't see a way to configure this :(
Flags: needinfo?(gkruglov)
Pff, when I disable the GPU in the AVD's .ini file(s), Fennec crashes. I ordered an HTC One through 'The Hub', but that seems to be stuck and not going anywhere.
At this point I have to leave this bug and leave it to someone else to fix. I'm fully prepared to mentor and review patches, though!
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(gkruglov)
Iteration: 51.3 - Sep 19 → ---
Points: 2 → ---
(In reply to Mike de Boer [:mikedeboer] from comment #19)
> At this point I have to leave this bug and leave it to someone else to fix.

Sorry, that's not really acceptable. I'm more than willing to just back out the regressing bug and anything else that depends on it, and call it a day.

IIRC you only ordered the device a day or two ago, have you tried pinging somebody to move it along? Also, did you ever get the emulator working on Linux?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #20)
> (In reply to Mike de Boer [:mikedeboer] from comment #19)
> > At this point I have to leave this bug and leave it to someone else to fix.
> 
> Sorry, that's not really acceptable. I'm more than willing to just back out
> the regressing bug and anything else that depends on it, and call it a day.

You're right. I was simply frustrated about the amount of time wasted and flipping tables, I guess.

> IIRC you only ordered the device a day or two ago, have you tried pinging
> somebody to move it along? Also, did you ever get the emulator working on
> Linux?

I haven't tried Linux. Will do that soon.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Thanks. I realize it can be very frustrating when things don't just work. If you need me to run a custom build with extra logging in the meantime I'm happy to do that.
See Also: → 1306490
(In reply to Mike de Boer [:mikedeboer] from comment #18)
> I turned on GL tracing to logcat and get the following output:
> 
> https://pastebin.mozilla.org/8914062
> 
> Grisha, how do I get to the emulator settings? I don't have Android Studio
> installed, just mozilla-central cloned & built. When I start the emulator
> with ./mach run I don't see a way to configure this :(

I access them via AVD Manager, which I access from Android Studio... You can launch a (differently skinned one?) via command line: .android-sdk/tools/android avd and I see graphics settings in there as well.
Guys, we are at the middle of the beta cycle. So, we need to come up with a solution quickly.
We can always backout bug 1281421 but we will need to make that call before beta 6 (gtb next Monday) to have time to find any new regression.
Flags: needinfo?(mdeboer)
Backing out bug 1281421 will create an even bigger mess, because all the findbar patches after that will need to be backed out as well.
The solution will be simple and the patch as well - the Finder.jsm module is used in slightly the wrong way. As an aside, I don't understand why I was never consulted when this functionality landed in the first place. But okay, Doesn't guarantee no bug slips in ever.
An android device is on its way and should arrive today. That makes things a lot easier. Please don't interpret the inactivity here as unwillingness to work on this from my end.
Flags: needinfo?(mdeboer)
> Please don't interpret the inactivity here as unwillingness to work on this from my end.
This wasn't my intention. Sorry if I suggested that. I was just going through the list of tracked bugs to make sure we are making progress.
(In reply to Sylvestre Ledru [:sylvestre] from comment #26)
> This wasn't my intention. Sorry if I suggested that. I was just going
> through the list of tracked bugs to make sure we are making progress.

No, I didn't. I just received the phone, so expect progress here soon!
Comment on attachment 8798549 [details]
Bug 1303008 - refactor Fennec Finder.jsm integration to always show the correct match count results in the findbar.

Nick, are you comfortable reviewing this or if not, do you know an appropriate reviewer?
Attachment #8798549 - Flags: review?(mark.finkle) → review?(nalexander)
Comment on attachment 8798549 [details]
Bug 1303008 - refactor Fennec Finder.jsm integration to always show the correct match count results in the findbar.

https://reviewboard.mozilla.org/r/83990/#review82632

This code is mostly not owned, so rather than redirecting to sebastian, I've looked at it.  r- only because I have one substantive question which we should address before landing.

General question: it seems like we have a layer of indirection around the toolkit-level "Finder:Matches*" and the Fennec-level "FindInPage:Matches*" messages.  Is that really necessary?  (It might be, with the limit and other things be stored in this module.)

::: mobile/android/base/java/org/mozilla/gecko/FindInPageBar.java:120
(Diff revision 1)
> -        EventDispatcher.getInstance().unregisterGeckoThreadListener(this, "TextSelection:Data");
> +        EventDispatcher.getInstance().unregisterGeckoThreadListener(this,
> +            "TextSelection:Data",
> +            "FindInPage:MatchesCountResult");
> +    }
> +
> +    private void onMatchesCountResult(final int total, final int current, final int limit, final String searchString) {

Not worth commenting, but I just want to be sure you're clear -- this is called on the Gecko thread (registerGeckoThreadListener), and then updates the UI on the UI thread.  That should be fine.

::: mobile/android/base/java/org/mozilla/gecko/FindInPageBar.java:192
(Diff revision 1)
>  
>      // GeckoEventListener implementation
>  
>      @Override
>      public void handleMessage(String event, JSONObject message) {
> +        if (event.equals("FindInPage:MatchesCountResult")) {

In general, prefer `"CONSTANT".equals(thing_that_could_be_null)`, because this won't give a `NullPointerException` if thing is in fact null.  However, the style here is not that -- probably 'cuz these inputs are always non-null (and trusted).

::: mobile/android/chrome/content/FindHelper.js:96
(Diff revision 1)
>     * Request, wait for, and return the current matchesCount results for a string.
>     */
>    _getMatchesCountResult: function(findString) {
>      // Count matches up to any provided limit.
>      if (this._limit <= 0) {
> -      return { total: 0, current: 0, limit: 0 };
> +      Messaging.sendRequest({ type: "FindInPage:MatchesCountResult", total: 0, current: 0, limit: 0 });

This transformation I don't follow.  This used to synchronously bail out, but now kicks off an async process.  (In all cases.)  At best the comment (Request, wait for, ...) is wrong.  But perhaps the function should be renamed -- or perhaps just folded in as a special case of `_finder.requestMatchesCount`?
Attachment #8798549 - Flags: review?(nalexander) → review-
(In reply to Nick Alexander :nalexander from comment #30)
> This code is mostly not owned, so rather than redirecting to sebastian, I've
> looked at it.  r- only because I have one substantive question which we
> should address before landing.

Thanks, Nick! And super-fast too :)

> General question: it seems like we have a layer of indirection around the
> toolkit-level "Finder:Matches*" and the Fennec-level "FindInPage:Matches*"
> messages.  Is that really necessary?  (It might be, with the limit and other
> things be stored in this module.)

Yeah, the `limit` addition is the only indirection, really, since the searchString is added for convenience to check if the match count result belongs to an empty string by any chance.
Both are used, apparently, to construct the string displayed in the UI next to the findbar input field.

> This transformation I don't follow.  This used to synchronously bail out,
> but now kicks off an async process.  (In all cases.)  At best the comment
> (Request, wait for, ...) is wrong.  But perhaps the function should be
> renamed -- or perhaps just folded in as a special case of
> `_finder.requestMatchesCount`?

The fact that this was assumed to be synchronous is wrong, but at the time this module was written this fact was interwoven with the findbar.xml binding code and quite obscure. So I don't blame anyone at all here ;-)
Much effort has been put lately in moving more and more code out of the binding into the JS modules, which incidentally caused this regression.
In any case, this is now asynchronous and should always be treated as such. Read `requestMatchesCount()` as if it's kicking off a worker which searches the page and comes back with the results a while later.
So while the findbar is open, the matchesCountResult JSON blobs will get sent to it and will be up-to-date. That's what was causing this bug, btw; since the only results that were processed were the ones returned by the `doFind()` or `findAgain()` methods, they were only updated later on at the next call. Intermediate results were skipped. Or old, invalid results were returned instead.

I think renaming the function and updating the comment would work here.
In this iteration I went a little bit further and ousted the `_getMatchesCountResult` method altogether. I added docblock comments above each method, because well, it doesn't hurt.
MozReview mentions this too, but I moved `onMatchesCountResult` to sit next to `onFindResult`, to make it more obvious they're both invoked by the Finder instance.

Apart from this patch & bug: it's a bit sad that this code is not (mostly) owned by one person - I'd be happy to step up here as indefinite maintainer and owner if the FindInPageBar. If that's at all a thing in Fennec-land.
Comment on attachment 8798549 [details]
Bug 1303008 - refactor Fennec Finder.jsm integration to always show the correct match count results in the findbar.

https://reviewboard.mozilla.org/r/83990/#review82830

::: mobile/android/chrome/content/FindHelper.js:109
(Diff revisions 1 - 2)
>      Messaging.removeListener("FindInPage:Next");
>      Messaging.removeListener("FindInPage:Prev");
>    },
>  
>    /**
> -   * Request, wait for, and return the current matchesCount results for a string.
> +   * Start a find-in-page operation, using the current Finder instance and request

Perhaps include "asynchronous" or "request" here.

::: mobile/android/chrome/content/FindHelper.js:126
(Diff revisions 1 - 2)
>      this._finder.fastFind(searchString, false);
> -    this._getMatchesCountResult(searchString);
> +    this._finder.requestMatchesCount(searchString, this._limit);
>      return { searchString, findBackwards: false };
>    },
>  
> +  /**

nit: via `doFind()`.  If we haven't called `doFind()`, ...
Attachment #8798549 - Flags: review?(nalexander) → review-
Comment on attachment 8798549 [details]
Bug 1303008 - refactor Fennec Finder.jsm integration to always show the correct match count results in the findbar.

https://reviewboard.mozilla.org/r/83990/#review82832

This looks good to me.
Attachment #8798549 - Flags: review- → review+
> Apart from this patch & bug: it's a bit sad that this code is not (mostly)
> owned by one person - I'd be happy to step up here as indefinite maintainer
> and owner if the FindInPageBar. If that's at all a thing in Fennec-land.

For code ownership, the people most associated with this code, margaret and capella, have left MoCo and stopped contributing, respectively.  This is classic "you touched it, you own it" territory.

Since there's no "official ownership" of this piece of Fennec, but I expect sebastian would be happy to have you as the principle reviewer, I'm NIing him for you to discuss further.  Thanks, Mike!
Flags: needinfo?(s.kaspari)
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a2c1d4e7e5e0
refactor Fennec Finder.jsm integration to always show the correct match count results in the findbar. r=nalexander
https://hg.mozilla.org/mozilla-central/rev/a2c1d4e7e5e0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Fix verified on Nightly. Thanks Mike! Please request uplift to Aurora and beta as well.
Status: RESOLVED → VERIFIED
Comment on attachment 8798549 [details]
Bug 1303008 - refactor Fennec Finder.jsm integration to always show the correct match count results in the findbar.

Separate patches are needed for landing on m-a and m-b, so I'll take care of it.

Approval Request Comment
[Feature/regressing bug #]: bug 1281421.
[User impact if declined]: User will see an incorrect count/ state in the find toolbar when using it on Fennec.
[Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass.
[Risks and why]: moderate, since I had to rework a bit of the code to make the Finder.jsm integration work correctly. The end result, however, is leaner.
[String/UUID change made/needed]: n/a.
Attachment #8798549 - Flags: approval-mozilla-beta?
Attachment #8798549 - Flags: approval-mozilla-aurora?
Thank you for reviewing the patch, nalexander!

(In reply to Nick Alexander :nalexander from comment #36)
> > Apart from this patch & bug: it's a bit sad that this code is not (mostly)
> > owned by one person - I'd be happy to step up here as indefinite maintainer
> > and owner if the FindInPageBar. If that's at all a thing in Fennec-land.
> 
> For code ownership, the people most associated with this code, margaret and
> capella, have left MoCo and stopped contributing, respectively.  This is
> classic "you touched it, you own it" territory.
> 
> Since there's no "official ownership" of this piece of Fennec, but I expect
> sebastian would be happy to have you as the principle reviewer, I'm NIing
> him for you to discuss further.  Thanks, Mike!

Oh, yeah, definitely! That would be incredibly helpful!
Flags: needinfo?(s.kaspari)
Comment on attachment 8798549 [details]
Bug 1303008 - refactor Fennec Finder.jsm integration to always show the correct match count results in the findbar.

New regression in 50, fix was verified in Nightly52, Aurora51+, Beta50+
Attachment #8798549 - Flags: approval-mozilla-beta?
Attachment #8798549 - Flags: approval-mozilla-beta+
Attachment #8798549 - Flags: approval-mozilla-aurora?
Attachment #8798549 - Flags: approval-mozilla-aurora+
has problems applying to aurora:

grafting 368792:a2c1d4e7e5e0 "Bug 1303008 - refactor Fennec Finder.jsm integration to always show the correct match count results in the findbar. r=nalexander"
merging mobile/android/base/java/org/mozilla/gecko/FindInPageBar.java
merging toolkit/content/widgets/findbar.xml
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/FindInPageBar.java! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(mdeboer)
Verified as fixed on both Aurora(51.0a2 / 2016-10-18) and Beta (50.0b8) on a Samsung Galaxy S6 Edge(Android 6.0) and Sony Xperia Z2 tablet(Android 5.1.1)
> Whereas in your logcat you get the shader compilation failure at this point.
> The shader in question lives at [1] and from the error message in your log
> ("Valid GLSL but not GLSL ES") it sounds like maybe GL_ES isn't defined?

For other folks seeing this message, I filed Bug 1318820.
See Also: → 1318820
Based on comment 47 I will remove the qe-verify flag, thanks.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.