Closed Bug 1140512 Opened 9 years ago Closed 9 years ago

(e10s) FindBar stops communicating properly with content after remoteness change

Categories

(Toolkit :: Find Toolbar, defect)

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

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
e10s m8+ ---
firefox44 --- verified

People

(Reporter: quicksaver, Assigned: yarik.sheptykin, Mentored)

References

Details

(Whiteboard: [good next bug][lang=js])

Attachments

(1 file, 8 obsolete files)

Ensure FindBar communicates properly with content after remoteness change
14.20 KB, patch
yarik.sheptykin
: review+
Details | Diff | Splinter Review 
STR:

1. Open Nightly, open about:home
2. Ctrl+F to open findbar
3. Find "e" or something, then hit F3 a few times
3.1 Phrase is found and match counter is updated correctly
4. Find "asdf"
4.1 Obviously it's not found, and findbar shows the reddish "Not Found" UI
5. Open about:addons in the same tab
6. Open about:home again
7. Try finding "e" again

Result: it visually finds the text in the page, but fails to update the counter. If you also find "asdf" or something else that doesn't exist, the findbar doesn't change to the "Not found" UI.

This happens because loading about:addons triggers a remoteness change [1], which in turn switches the binding on the browser element and re-nulls its ._finder and ._remoteFinder properties, without reinitializing the findbar with it as it does when it is first created [2][3].

Easiest fix would probably be to listen for the TabRemotenessChange event and destroy the findbar so it is recreated from scratch when it is needed next, unless there are non-remote tabs where it can be used? I don't remember any from the top of my head.

Best would probably be to just reinitialize the same findbar if it already exists with the new finder. BTW, I don't think it's *just* the Finder that needs to be reinitialized, as when the remoteness changes the browser is .destroy()'ed, which I *think* also destroys the findbar's system event listeners for 'keypress' and 'mouseup' [4], although I haven't tested this.

In either case, bonus points for refactoring the findbar destruction code into its own method [5] and/or send an event for when a findbar is destroyed/reinitialized, just like it happens when it is first created [6], so that add-ons like mine can react properly. :)

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1475
[2] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#179
[3] http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#256
[4] http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#280
[5] http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#269
[6] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#202
Tom, CC'ing you because you worked on this first way back in bug 666816, thought you might want to know or have more insights about this.
(In reply to Luís Miguel [:Quicksaver] from comment #0)
> Best would probably be to just reinitialize the same findbar if it already
> exists with the new finder.

By this I meant re-assign the findbar's .browser property should be enough.
Mentor: felipc
Whiteboard: [good next bug][lang=js]
I'm not sure if this bug will still need to be fixed once I finish with bug 1133981...
Luís, is this bug still happening with current nightly now that bug 1133981 is fixed?
Flags: needinfo?(quicksaver)
(In reply to :Gijs Kruitbosch from comment #4)
> Luís, is this bug still happening with current nightly now that bug 1133981
> is fixed?

Yep, tried in today's nightly, got the same behavior as I reported originally.
Flags: needinfo?(quicksaver)
Assignee: nobody → mrbkap
Hi Guys!

Sorry for intervening, I know this bug is assigned to :mrbkap. I just found it very interesting and could not stop myself from trying to understand what was going on. It all started with an attempt to reproduce the issue (which succeeded) and ended with porting browser_findbar tests to e10s. If you :mrbkap find this patch helpful please feel free to build upon it. I would also be happy to continue working on this if you are OK with it.
Attachment #8633451 - Flags: review?(mrbkap)
(In reply to Iaroslav Sheptykin from comment #6)
> Created attachment 8633451 [details] [diff] [review]
> 377 	gBrowser.tabContainer.addEventListener("TabRemotenessChange",
>         this.onRemotenessChange.bind(this));

This bind() makes it impossible to remove this listener. For instance in my add-on there's a specific situation where I have to destroy the findbar and recreate it (true that it's a bit of an ugly thing to do, but until now I couldn't find any way around it), and that bind() would cause it to live forever in memory until the tabContainer itself is destroyed, which of course never happens.

Is there no other way to do this? Maybe with a handleEvent method instead, and add the findbar itself as the listener?
BTW that will actually end up being necessary to remove that listener in the findbar's own destroy method, which I believe should happen.
(In reply to Luís Miguel [:Quicksaver] from comment #7)

Hey Luis! Thanks for looking into the patch!

> Is there no other way to do this? Maybe with a handleEvent method instead,
> and add the findbar itself as the listener?

Sure, I can attach the listener code in another way.

(In reply to Luís Miguel [:Quicksaver] from comment #8)
> BTW that will actually end up being necessary to remove that listener in the
> findbar's own destroy method, which I believe should happen.

Good point.
Iaroslav, thanks for looking at this! I'm assigning the bug to you. When you write your next patch, though, you should ask mdeboer@mozilla.com for review since he owns the code in question.
Assignee: mrbkap → yarik.sheptykin
Comment on attachment 8633451 [details] [diff] [review]
Ensure FindBar communicates properly with content after remoteness change

Review of attachment 8633451 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing this request for now.

::: toolkit/content/widgets/findbar.xml
@@ +383,5 @@
>  
> +      <method name="onRemotenessChange">
> +        <body><![CDATA[
> +          this.browser._lastSearchString = this._findField.value;
> +          this.browser = this.browser;

It'd be good to add a comment noting that on a remoteness change the identity of the browser stays the same but we need to re-initialize our listeners.
Attachment #8633451 - Flags: review?(mrbkap)
Hi guys!
Here is an updated version of the patch. Could you take a look and tell me if it is going in the right direction?
Attachment #8633451 - Attachment is obsolete: true
Attachment #8637179 - Flags: review?(mdeboer)
(In reply to Iaroslav Sheptykin from comment #12)
> Created attachment 8637179 [details] [diff] [review]
> Ensure FindBar communicates properly with content after remoteness change

handleEvent doesn't need to be binded, that is done automatically. So instead of a .onRemotenessChange method, you can actually create the following:

> <method name="handleEvent">
>   <parameter name="aEvent"/>
>   <body><![CDATA[
>     switch(aEvent.type) {
>       case 'TabRemotenessChange':
>         // Reinitializing browser to re-attach listeners.
>         this.browser._lastSearchString = this._findField.value;
>         this.browser = this.browser;
>         break;
>     }
>   ]]></body>
> </method>

The switch helps safeguard for the future if the findbar listens to other events later, they'll be easier to add, although for now it's irrelevant as the findbar object doesn't listen for anything else.

And in the constructor/destructor add/remove the object directly:

> gBrowser.tabContainer.addEventListener("TabRemotenessChange", this);

I just mention this because I find handleEvent() is awesome for easily assigning "this" to an object. Maybe others will disagree? :)

I can't find a lot of good documentation on this, but here's one:
http://www.thecssninja.com/javascript/handleevent

And a quick example in firefox code:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/socialmarks.xml#309
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/socialmarks.xml#65
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/socialmarks.xml#347
Comment on attachment 8637179 [details] [diff] [review]
Ensure FindBar communicates properly with content after remoteness change

Review of attachment 8637179 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this, Iaroslav!

I'm sure this was working for you locally at some point, but I can't see how in this version :(

Can you take a look?

::: toolkit/content/tests/browser/browser_findbar.js
@@ +1,5 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "Promise",
>    "resource://gre/modules/Promise.jsm");
>  Components.utils.import("resource://gre/modules/Timer.jsm", this);
>  
> +function promiseKeypress(browser, key){

We have a module that already defines most of the helper functions you define in this file, except for simulating key-presses: https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm

It's frame-script (content) counterpart is located here: https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/AsyncUtilsContent.js

So you'll need to add a `sendChar` function to BrowserTestUtils.jsm, instead of defining it here. You can load URIs and add tabs with the functions already defined in that file.

@@ +31,5 @@
> + * process into the parent process or the other way around.
> + * This test ensures that findbar properly handles such a change.
> + */
> +add_task(function * test_reinitialization_at_remoteness_change() {
> +

nit: superfluous newline.

@@ +51,5 @@
> +  yield promiseFindFinished("a", false);
> +  ok(!findbar._findStatusDesc.textContent, "Findbar status should be empty");
> +
> +  gBrowser.removeTab(tab);
> +

nit: superfluous newline.

@@ +61,4 @@
>   * by calling stopPropagation on a keypress event on a page.
>   */
>  add_task(function* test_hotkey_event_propagation() {
> +

nit: superfluous newline.

@@ +98,5 @@
>      yield closeFindbarAndWait(findbar);
>    }
>  
>    gBrowser.removeTab(tab);
> +

nit: superfluous newline.

::: toolkit/content/widgets/findbar.xml
@@ +382,5 @@
>        ]]></destructor>
>  
> +      <method name="onRemotenessChange">
> +        <body><![CDATA[
> +          handleEvent = () => {

So what you're doing here is:
1) 'TabRemotenessChange' event fires;
2) The 'onRemotenessChange' method is invoked;
3) It defines a function called 'handleEvent';
4) Exits without doing anything.

I don't think that will work... have you tested this patch?
Attachment #8637179 - Flags: review?(mdeboer)
Attached patch bug1140512.patch (obsolete) — — Splinter Review 
Hi Mike!

Thanks for taking a look at my patch!

> ::: toolkit/content/widgets/findbar.xml
> @@ +382,5 @@
> >        ]]></destructor>
> >  
> > +      <method name="onRemotenessChange">
> > +        <body><![CDATA[
> > +          handleEvent = () => {
> 
> So what you're doing here is:
> 1) 'TabRemotenessChange' event fires;
> 2) The 'onRemotenessChange' method is invoked;
> 3) It defines a function called 'handleEvent';
> 4) Exits without doing anything.
> 
> I don't think that will work... have you tested this patch?

This worked for me. As far as I understand handleEvent [1], the event listener [2] is an interface having one method handleEvent(event). handleEvent will be called on whatever you pass as the enventListener into addEventListener(). If you pass a function its default implementation of handleEvent just executes the function body [1]. At least this is how I interpret mdn docs on this matter. But I changed the code to use one handleEvent for all events, registering 'this' as a listener.

All tests pass on my machine. If the patch looks allright I can run it trough the tryserver.

1. https://developer.mozilla.org/en-US/docs/Web/API/EventListener
2. http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-EventListener
Attachment #8637179 - Attachment is obsolete: true
Attachment #8642158 - Flags: review?(mdeboer)
Comment on attachment 8642158 [details] [diff] [review]
bug1140512.patch

>+  sendChar(browser, aChar) {
>+    return new Promise(resolve => {
>+      let mm = browser.messageManager;
>+      mm.addMessageListener("Test:SendCharDone", function charMsg(message) {
>+        mm.removeMessageListener("Test:SendCharDone", charMsg);
>+        resolve(message.data.sendCharResult);
>+      });
>+
>+      mm.sendAsyncMessage("Test:SendChar", {aChar : aChar});
>+    });

Personally, I'd rather you keep the original form and require the window to be supplied, then get the selected browser from that or 'window' if not supplied, although it ends up sending the key to the browser anyway, so perhaps it doesn't end up mattering too much. Either way, please do keep the original ordering of the arguments to sendChar, and have 'aChar' the first argument and 'aBrowser' the second argument.

(aside: the original test is actually written incorrectly; it shouldn't pass the window argument to sendChar at all)
Comment on attachment 8642158 [details] [diff] [review]
bug1140512.patch

Review of attachment 8642158 [details] [diff] [review]:
-----------------------------------------------------------------

First off: apologies for not getting to this sooner - I was on holiday for two weeks!

This is getting very close! Thanks for making this good progress! :-)

Like you mentioned, a push to Try is quite a good idea! Let's do that with your next version of this patch. Please let me know if you need help!

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +451,5 @@
>      });
> +  },
> +
> +  /**
> +   *  Versions of EventUtils.jsm sendChar functions that synthesize a

nit: indentation of text.

@@ +454,5 @@
> +  /**
> +   *  Versions of EventUtils.jsm sendChar functions that synthesize a
> +   *  keypress event in a child process and return promises that resolve when the
> +   *  event has fired and completed. Instead of a window, a browser is required
> +   *  to be passed to this function.

nit: please rewrite this to:

'Version of EventUtils' `sendChar` function; it will synthesize a keypress event in a child process and returns a Promise that will result when the event was fired. Instead of a Window, a Browser object is required to be passed to this function.'

@@ +457,5 @@
> +   *  event has fired and completed. Instead of a window, a browser is required
> +   *  to be passed to this function.
> +   *
> +   * @param {Browser} browser
> +   *        Browser element, must not be null

nit: missing dot.

@@ +459,5 @@
> +   *
> +   * @param {Browser} browser
> +   *        Browser element, must not be null
> +   * @param {string} aChar
> +   *        A character for the kezpress event that is sent to the browser

nit: s/kezpress/keypress, missing dot.

@@ +462,5 @@
> +   * @param {string} aChar
> +   *        A character for the kezpress event that is sent to the browser
> +   *
> +   * @returns {Promise}
> +   * @resolves True if the kezpress event was synthesized.

nit: s/kezpress/keypress

@@ +464,5 @@
> +   *
> +   * @returns {Promise}
> +   * @resolves True if the kezpress event was synthesized.
> +   */
> +  sendChar(browser, aChar) {

`aChar` is deprecated Hungarian notation for 'argument called char'. Please make this say simply `char`.
Also, I think Neil is right in comment 16, that swapping the arguments here is better.

@@ +472,5 @@
> +        mm.removeMessageListener("Test:SendCharDone", charMsg);
> +        resolve(message.data.sendCharResult);
> +      });
> +
> +      mm.sendAsyncMessage("Test:SendChar", {aChar : aChar});

nit: please format this as `{ char: char }`

::: testing/mochitest/tests/SimpleTest/AsyncUtilsContent.js
@@ +10,5 @@
>  EventUtils.window = {};
>  EventUtils.parent = EventUtils.window;
>  EventUtils._EU_Ci = Components.interfaces;
>  EventUtils._EU_Cc = Components.classes;
> +EventUtils.navigator = content.document.defaultView.navigator;

Do you absolutely need this? If so, please add a comment above stating why.

@@ +43,5 @@
>    sendAsyncMessage("Test:SynthesizeMouseDone", { defaultPrevented: result });
>  });
> +
> +addMessageListener("Test:SendChar", message => {
> +  let result = EventUtils.sendChar(message.data.aChar, content);

Please make this `message.data.char`.

@@ +44,5 @@
>  });
> +
> +addMessageListener("Test:SendChar", message => {
> +  let result = EventUtils.sendChar(message.data.aChar, content);
> +  sendAsyncMessage("Test:SendCharDone", {sendCharResult : result});

nit: please format this as: `{ sendCharResult: result }`, which is how we generally format object literals on a single line.

::: toolkit/content/tests/browser/browser_findbar.js
@@ +10,5 @@
> + * 'Remoteness change' means that rendering page content moves from child
> + * process into the parent process or the other way around.
> + * This test ensures that findbar properly handles such a change.
> + */
> +add_task(function * test_reinitialization_at_remoteness_change() {

Generally, we put test cases at the bottom of the file, below the last `add_task()` in this case.
Can you do that here too?

@@ +23,5 @@
> +  yield promiseFindFinished("s", false);
> +  ok(!findbar._findStatusDesc.textContent, "Findbar status should be empty");
> +
> +  // Navigating to "about:addons" triggers a change of browser's remoteness.
> +  yield BrowserTestUtils.loadURI(browser, "about:addons");

I don't think it's OK to rely on 'about:addons' to not be loaded in an e10s tab. Instead, I'd recommend to load the same `TEST_PAGE_URI` in a new non-e10s tab.

@@ +30,5 @@
> +  // Findbar should keep operating normally.
> +  yield promiseFindFinished("a", false);
> +  ok(!findbar._findStatusDesc.textContent, "Findbar status should be empty");
> +
> +  gBrowser.removeTab(tab);

Please use `yield BrowserTestUtils.removeTab(tab);`

@@ +160,5 @@
> +      if (highlightOn) {
> +        // When highlight all is set another message will be sent after search is
> +        // wrapped. With e10s this leands to timing problems. So here we give
> +        // findbar some time to do proper message handling.
> +        setTimeout(() => deferred.resolve(), 1000);

Please find a way to prevent setting this timeout. For example, when another message is sent when 'Highlight All' is turned on, you'll have to wait for that event to finish as well.

::: toolkit/content/widgets/findbar.xml
@@ +381,5 @@
>          this.destroy();
>        ]]></destructor>
>  
> +      <method name="handleEvent">
> +      <parameter name="aEvent"/>

nit: please indent this properly.

@@ +421,5 @@
> +          gBrowser.tabContainer.removeEventListener("TabRemotenessChange", this);
> +
> +          // Some add-ons might be interested in reacting to this event.
> +          let event = new Event("TabFindDestroyed");
> +          gBrowser.tabContainer.dispatchEvent(event);

This isn't the way to do this.

In fact, why don't you just remove this code? Add-ons can listen to when a tab closes, which'll destroy the findbar instance.

Let's add code when there's a use-case for it.
Attachment #8642158 - Flags: review?(mdeboer) → feedback+
(In reply to Mike de Boer [:mikedeboer] from comment #17)
> @@ +421,5 @@
> > +          gBrowser.tabContainer.removeEventListener("TabRemotenessChange", this);
> > +
> > +          // Some add-ons might be interested in reacting to this event.
> > +          let event = new Event("TabFindDestroyed");
> > +          gBrowser.tabContainer.dispatchEvent(event);
> 
> This isn't the way to do this.

FWIW it seems on par with the event sent when the findbar is created, except that is dispatched from the tab itself and not from tabContainer: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#173

> In fact, why don't you just remove this code? Add-ons can listen to when a
> tab closes, which'll destroy the findbar instance.

I purposed this event because I'm interested in it for my add-on FindBar Tweak. As long as closing the tab is the only instance when a findbar is destroyed, I'm fine with it (haven't researched this to be sure yet, but I definitely will later today).
Hi Guys!

Sorry for taking this long for updating the patch. Since recently I have little free time. Anyways, here comes a new version of the patch with the feedback you guys gave me addressed. Please pay extra attention to a construct with waitMore in promiseFindFinished (browser_findbar.js test). This looks like a hack to me but I cant come up with a more elegant solution without digging too deep into the finder code (which might take a while). 
If this looks more or less ok I would push the patch to the tryserver. Let me know if I should.
Attachment #8642158 - Attachment is obsolete: true
Attachment #8655384 - Flags: review?(mdeboer)
Yep, definitely send it to tryserver :)
The results don't look good :( 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f99b6722d3a

Investigating ... and back to coding.
Comment on attachment 8655384 [details] [diff] [review]
Ensure FindBar communicates properly with content after remoteness change

Review of attachment 8655384 [details] [diff] [review]:
-----------------------------------------------------------------

Cancelling review in that case! :) And yeah, it's always a good idea to push to try... especially since your patches are going into the final rounds of review.
Attachment #8655384 - Flags: review?(mdeboer)
Hi guys!

Here is the new version of the patch. I investigated the test failures fixed the code and resent it to the tryserver. The problems were related to undefined 'gBrowser' in some specific chrometest and to undefined 'navigator' in EventUtils.

The tests seem to look good now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f71df1c02427
Attachment #8655384 - Attachment is obsolete: true
Attachment #8658638 - Flags: review?(mdeboer)
Comment on attachment 8658638 [details] [diff] [review]
Ensure FindBar communicates properly with content after remoteness change

Review of attachment 8658638 [details] [diff] [review]:
-----------------------------------------------------------------

OK, this looks really good, Iaroslav!! Thanks :)

Can you upload an updated patch with my comments addressed and a link to the try build with that patch applied? Thanks in advance :)

I didn't get to reviewing your patch in time because I was out sick for two weeks, unfortunately... :'( Even though, I apologize for the delay once more!

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +452,5 @@
>      });
> +  },
> +
> +  /**
> +   * 'Version of EventUtils' `sendChar` function; it will synthesize a keypress

nit: please remove this first single quote? (s/'Version/Version/)

@@ +460,5 @@
> +   *
> +   * @param {Browser} browser
> +   *        Browser element, must not be null.
> +   * @param {string} aChar
> +   *        A character for the keypress event that is sent to the browser.

nits: you change the order of the arguments, so please change it here too. Also, can you rename `aChar` to `char` here?

@@ +473,5 @@
> +        mm.removeMessageListener("Test:SendCharDone", charMsg);
> +        resolve(message.data.sendCharResult);
> +      });
> +
> +      mm.sendAsyncMessage("Test:SendChar", { char : char });

nit: change this to `{ char: char }`.

::: testing/mochitest/tests/SimpleTest/AsyncUtilsContent.js
@@ +10,5 @@
>  EventUtils.window = {};
>  EventUtils.parent = EventUtils.window;
>  EventUtils._EU_Ci = Components.interfaces;
>  EventUtils._EU_Cc = Components.classes;
> +EventUtils.navigator = content.document.defaultView.navigator;

I think I asked for a comment here before...

@@ +44,5 @@
>  });
> +
> +addMessageListener("Test:SendChar", message => {
> +  let result = EventUtils.sendChar(message.data.char, content);
> +  sendAsyncMessage("Test:SendCharDone", { sendCharResult : result });

nit: change this to `{ sendCharResult: result }`.
Attachment #8658638 - Flags: review?(mdeboer) → review+
Hi Mike!

Thanks for the feedback on the patch! Here is the new one with the changes you requested. Please take a look at my comment regrading 'navigator'. I am don't have a complete understanding of how EventUtils work, so the comment is a little vague. I you wish more clarity I can check the EventUtils code to get a better picture of what is going on there.

I submitted this patch to the tryserver but the tests are not over yet:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fad9f461c13
Attachment #8658638 - Attachment is obsolete: true
Attachment #8663406 - Flags: review+
Attachment #8663406 - Flags: feedback?(mdeboer)
Oh, the tests look very weird. Mac build seems to fail completely. I believe that this patch has nothing to do with that, but I might resend it to tryserver once more. Or what do you think?
Yeah those test problems look unrelated to your patch. I think they were just infra failures. I retriggered the OSX build. I'm not sure if that will only redo the build, or also run all the tests there, but in any case, I think this looks good.  The M-e10s(4) failure appears to be a known intermittent failure that is also unrelated, but I retriggered that test as well just in case.
Thanks Felipe! The tests look a better now. I am gonna wait for the feedback from Mike and than add a checkin-needed if he is ok with the patch.
Comment on attachment 8663406 [details] [diff] [review]
Ensure FindBar communicates properly with content after remoteness change

Review of attachment 8663406 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for addressing the comments, Iaroslav!

I'll go ahead and request checkin here.

::: testing/mochitest/tests/SimpleTest/AsyncUtilsContent.js
@@ +10,5 @@
>  EventUtils.window = {};
>  EventUtils.parent = EventUtils.window;
>  EventUtils._EU_Ci = Components.interfaces;
>  EventUtils._EU_Cc = Components.classes;
> +// EventUtils' `sendChar` function relies on the navigator to synthetize events.

Well, this is perfect for me :)
Attachment #8663406 - Flags: feedback?(mdeboer)
https://hg.mozilla.org/mozilla-central/rev/77b2f22f48c7
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Hmm... So, I can still reproduce this in the latest nightly. I temporarily can't use about:home as in the original report because of bug 1195038, but any other webpage will do.

I'm not sure yet why the patch doesn't work, but I think I know why the test isn't catching it. The remoteness change doesn't trigger a findbar status reset, so if the status was empty before, it's going to be empty forever (startFind won't trigger a status reset if the findbar is already open, which is the case in the test).

It's also probably not a good idea to only check for an empty status because a status reset means it'll be empty anyway. I would suggest:
- check for failed find, with "z",
- check for correct empty, with "s",
- remoteness change, (are we sure this is also happening? Maybe check if the browser is remote before and non-remote after as well, although we'd have to skip this test altogether in non-e10s for that)
- check for failed find, with "z",
- check for correct empty, with "s".

I also suggest a backout for now as the patch apparently isn't doing anything. :)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Well... I wonder how none of us noticed this. The event is TabRemotenessChange, not onRemotenessChange. :) (in handleEvent's switch block).

BTW I just noticed that with the listener attached to the tabContainer, the find bar will react to remoteness changes from other tabs, which is unnecessary, only the find bar from the tab that switched remoteness needs to reinitialize. I would either add the listener to the specific tab rather than to the tabContainer, or add a check in handleEvent like |if(event.target._findBar != this) return;|
Hey Luis! Thanks for for checking upon this fix. It is very unfortunate that the patch appeared useless. I am looking into it. Might take a couple of days, because I already started some other work.
https://hg.mozilla.org/integration/fx-team/rev/3581bfec80da83d207d55dda976087afce57a27e
Backed out changeset 77b2f22f48c7 (bug 1140512) due to invalid fix. rs=me,backout.
Status: REOPENED → ASSIGNED
Hi there! Sorry for a long delay. It was hard to find free time lately to keep firefox awesome. Here is a new version of the patch, with (hopefully) all feedback addressed. 
Please double-check the place where I load sample page content with 'javascript:' after remoteness change. Seems like a hack. Felipe, is there a nicer solution here?
Attachment #8663406 - Attachment is obsolete: true
Flags: needinfo?(felipc)
Attachment #8671849 - Flags: review?(mdeboer)
(In reply to Iaroslav Sheptykin from comment #38)
> Created attachment 8671849 [details] [diff] [review]
> Ensure FindBar communicates properly with content after remoteness change

Question: is it necessary to add/remove the listener on every |browser| assignment? I really don't know nor have tested, but I was wondering if it wouldn't be enough to set the listener only when the findbar is created, and remove it when it's destroyed. The tab itself is unaffected on remoteness change, and a tab and its findbar should never fall out of sync like that (I believe likewise for Mike's comment 17 on not needing the TabFindDestroyed event).

Adding the listener in getFindBar in tabbrowser.xml when it's created would also remove the need to do it in toolkit, thus sparing the cycles of checking for gBrowser in each findbar.

Unless I'm wrong or it's somehow needed for tests? As I said, haven't tested.
(In reply to Luís Miguel [:quicksaver] from comment #39)
> Adding the listener in getFindBar in tabbrowser.xml when it's created would
> also remove the need to do it in toolkit, thus sparing the cycles of
> checking for gBrowser in each findbar.

Unless the point really is to keep everything-findbar inside findbar.xml, in which case disregard my previous comment. :)
(In reply to Luís Miguel [:quicksaver] from comment #39)

Hi Luis! Thanks for taking a look at the previous patch!

> Question: is it necessary to add/remove the listener on every |browser|
> assignment? I really don't know nor have tested, but I was wondering if it
> wouldn't be enough to set the listener only when the findbar is created, and
> remove it when it's destroyed. The tab itself is unaffected on remoteness
> change, and a tab and its findbar should never fall out of sync like that (I
> believe likewise for Mike's comment 17 on not needing the TabFindDestroyed
> event).

You are right. This isn't necessary. 

> Adding the listener in getFindBar in tabbrowser.xml when it's created would
> also remove the need to do it in toolkit, thus sparing the cycles of
> checking for gBrowser in each findbar.

I thought keeping all findbar related code in findbar.xml would be better. But I also feel uncomfortable using gBrowser within toolkit. As far as I understand toolkit is meant to be useful across different applications, some of which might not provide gBrowser. So I updated the patch by relocating the part that resets the findbar browser into the tabbrowser. This solution looks better to me. Thanks for the idea, Luis!
Attachment #8671849 - Attachment is obsolete: true
Attachment #8671849 - Flags: review?(mdeboer)
Attachment #8672056 - Flags: review?(mdeboer)
(In reply to Iaroslav Sheptykin from comment #41)
> Created attachment 8672056 [details] [diff] [review]
> Ensure FindBar communicates properly with content after remoteness change

I wonder if it would be better to reset .browser before the TabRemotenessChange event is dispatched. I think it makes more sense that listeners expect everything (i.e. the findbar) to be in working order when the process (i.e. the remoteness change) is/has finished/announced. (I'm not sure how to phrase this... let me know if you need me to be more explicit.)

For instance, if the test wasn't loading the test uri after the remoteness change was called, it would fail, as the test would proceed as soon as the promise is resolved (when the event is dispatched), before .browser was reset.

TL;DR:
You know what, that's not what I had suggested exactly, I meant to implement an actual event listener for TabRemotenessChange in gBrowser in the same fashion as the previous version of the patch for the findbar binding. I just came to this bug to post a follow-up suggestion to do it directly inside the updateBrowserRemoteness() method itself, which turns out is exactly what you did, so kudos for thinking further ahead! :)
(In reply to Luís Miguel [:quicksaver] from comment #42)

Hey Luis! 

> I wonder if it would be better to reset .browser before the
> TabRemotenessChange event is dispatched. I think it makes more sense that
> listeners expect everything (i.e. the findbar) to be in working order when
> the process (i.e. the remoteness change) is/has finished/announced. (I'm not
> sure how to phrase this... let me know if you need me to be more explicit.)
> 
> For instance, if the test wasn't loading the test uri after the remoteness
> change was called, it would fail, as the test would proceed as soon as the
> promise is resolved (when the event is dispatched), before .browser was
> reset.

Yeah, that definitely makes sense. It is not a fundamental change though, so I will wait for Mike's review and feedback from Felipe, and then update the patch again.
(In reply to Iaroslav Sheptykin from comment #38)
> Please double-check the place where I load sample page content with
> 'javascript:' after remoteness change. Seems like a hack. Felipe, is there a
> nicer solution here?

Looks good to me. If it's working, it's fine. We could monkey-patch canLoadURIInProcess, but as it's not necessary you can keep it this way. The biggest problem would be in the future when someone also block javascript: URIs from loading in the parent, which is a possibility. But then we can think of an alternative here
Flags: needinfo?(felipc)
Comment on attachment 8672056 [details] [diff] [review]
Ensure FindBar communicates properly with content after remoteness change

Review of attachment 8672056 [details] [diff] [review]:
-----------------------------------------------------------------

I think you got it this time :) Thanks!

And apology for the longer wait.

::: browser/base/content/tabbrowser.xml
@@ +1582,5 @@
>              let evt = document.createEvent("Events");
>              evt.initEvent("TabRemotenessChange", true, false);
>              tab.dispatchEvent(evt);
>  
> +            // If the findbar is up resetting its browser bindings.

nit: please rephrase this as: 'If the findbar has been initialised, reset its browser reference.'
Attachment #8672056 - Flags: review?(mdeboer) → review+
Hi Guys! Thanks for the feedback and review! I updated the patch and pushed it to the tryserver.
Attachment #8672056 - Attachment is obsolete: true
Attachment #8674224 - Flags: review+
Hi Guys! The tests seem good to me. Some failed but those look unrelated. Are we ready for another [checkin-needed], Mike?
Flags: needinfo?(mdeboer)
Indeed! Checked in. Thanks Iaroslav, Quicksaver and Mikedeboer for all the work on this bug :)
Flags: needinfo?(mdeboer)
https://hg.mozilla.org/mozilla-central/rev/799c8a9e33e0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
It appears this breaks another one of QuickSaver's add-ons, FindBar Tweak.  Also some Stylish styles that make changes to the Findbar are also broken.  So far what is broken is when you type in a search word it doesn't highlight any occurrence and clicking on the direction of search arrows doesn't highlight the word either nor does F3.  However the highlight button does work.
(In reply to Gary [:streetwolf] from comment #52)

Thanks for reporting that, Gary! The patch landed just a couple of days ago and I am imagining that :quicksaver is working on updating his addons right now. Is that so, :quicksaver?
If there is an issue with Firefox that makes fixing these addons impossible, I suggest opening another bug for that describing what is going wrong.
Flags: needinfo?(quicksaver)
I actually didn't expect this patch to break FBT at all. But it obviously does, just my luck...

Gary, I've uploaded a quick fix to https://github.com/Quicksaver/FindBar-Tweak/releases/tag/v2.1.3a1, but if you still have problems with it let me know as usual (https://github.com/Quicksaver/FindBar-Tweak/issues/228)
Flags: needinfo?(quicksaver)
On a cheerier note, the patch appears to work just fine in yesterday's Nightly. Following the steps from comment 0, I can no longer reproduce the issue. So, awesome. :)
Status: RESOLVED → VERIFIED
See Also: → 1216541
+  sendChar(char, browser) {
+    return new Promise(resolve => {
+      let mm = browser.messageManager;
+      mm.addMessageListener("Test:SendCharDone", function charMsg(message) {
+        mm.removeMessageListener("Test:SendCharDone", charMsg);
+        resolve(message.data.sendCharResult);
+      });
+      mm.sendAsyncMessage("Test:SendChar", { char: char });
+    });
+  }
 };

This isn't correct. Synthesized keys should be sent to the parent process which will send them to the child as needed. I filed bug 1216541 on this issue.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: