Closed Bug 1133981 Opened 9 years ago Closed 9 years ago

[e10s] Stop sending unsafe CPOWs after the findbar has been closed in a remote browser

Categories

(Toolkit :: Find Toolbar, defect)

Product:
Toolkit
Component:
Find Toolbar
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal
Points:
3

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla40
Iteration:
40.1 - 13 Apr
Tracking Flags:
Tracking Status
e10s m6+ ---
firefox40 --- verified

People

(Reporter: mconley, Assigned: Gijs)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

MozReview Request: Bug 1133981 - e10s-ify findbar FAYT key handling, r?mconley
39 bytes, text/x-review-board-request
mconley
: review+
Details 
+++ This bug was initially created as a clone of Bug #1133568 +++

STR:

1) In an e10s window, open any page that has a <textarea> or <input type="text">.
2) Open the find bar (Cmd-F on OS X, Ctrl-F elsewhere), and then close it again (Esc).
3) Start typing into one of the text inputs.

For each keypress, a ton of "unsafe CPOW usage" messages get sent through the Browser Console.

They're centered around two pieces of code:

from toolkit/modules/BrowserUtils.jsm:

  /**
   * Return the current focus element and window. If the current focus
   * is in a content process, then this function returns CPOWs
   * (cross-process object wrappers) that refer to the focused
   * items. Note that calling this function synchronously contacts the
   * content process, which may block for a long time.
   *
   * @param document The document in question.
   * @return [focusedElement, focusedWindow]
   */
  getFocusSync: function(document) {
    let elt = document.commandDispatcher.focusedElement;
    var window = document.commandDispatcher.focusedWindow;

    const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
    if (elt instanceof window.XULElement &&
        elt.localName == "browser" &&
        elt.namespaceURI == XUL_NS &&
        elt.getAttribute("remote")) {
      [elt, window] = elt.syncHandler.getFocusedElementAndWindow(); <-- Causes CPOW warning
    }

    return [elt, window];
  },

From toolkit/content/widgets/findbar.xml:

     <method name="_shouldFastFind">
        <parameter name="aEvent"/>
        <body><![CDATA[
          if (aEvent.ctrlKey || aEvent.altKey || aEvent.metaKey ||
              aEvent.defaultPrevented)
            return false;

          let {BrowserUtils} = Components.utils.import("resource://gre/modules/BrowserUtils.jsm", {});
          let [elt, win] = BrowserUtils.getFocusSync(document);

          if (elt) {
            if (elt instanceof HTMLInputElement && elt.mozIsTextField(false)) <-- Causes CPOW warning
              return false;

            if (elt.isContentEditable) <-- Causes CPOW warning
              return false;

            if (elt instanceof HTMLTextAreaElement || <-- Causes CPOW warning
                elt instanceof HTMLSelectElement ||
                elt instanceof HTMLObjectElement ||
                elt instanceof HTMLEmbedElement)
              return false;
          }

At the very least, we should stop sending down these CPOWs when the findbar has been closed.
Flags: firefox-backlog+
Points: --- → 3
Flags: qe-verify+
Assignee: nobody → mconley
Assignee: mconley → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 39.2 - 23 Mar
Attached file MozReview Request: bz://1133981/Gijs (obsolete) — 
/r/5207 - Bug 1133981 - e10s-ify findbar FAYT key handling, r?mconley

Pull down this commit:

hg pull review -r 6330f1ec0a54b2e931a25ef57e61b6c0bebc3b51
https://reviewboard.mozilla.org/r/5205/#review4213

::: toolkit/modules/Finder.jsm
(Diff revision 1)
> +    const HTMLAnchorElement = (node.ownerDocument || node).defaultView.HTMLAnchorElement;

Ran into this while testing, probably unrelated. This is a JSM, so the instanceof check 2 lines later here won't work if you don't define this.
Try push: remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=192cf3b678df

Note that the version on try missed adding the Mouseup handler in findbar.xml (it does try to remove it). All the tests passed locally, though, so I expect there won't be any real difference in the try outcome...
Just asking to clarify (as this might be important for my add-on), this patch removes the use of CPOWs completely right? And not just for the "after closing findbar" part. Or by passing the FindBar object to the nsIEventListenerService.addSystemEventListener method it still sends it to the chrome process as a CPOW?

BTW Gijs, noticed a little nit, toolkit/content/browser-content.js line 601, shouldFastFind method, there's one extra line break before the closing bracket, unless it was meant to be after the bracket to separate from the following _onKeypress method.
(In reply to Luís Miguel [:Quicksaver] from comment #4)
> Just asking to clarify (as this might be important for my add-on), this
> patch removes the use of CPOWs completely right? And not just for the "after
> closing findbar" part. Or by passing the FindBar object to the
> nsIEventListenerService.addSystemEventListener method it still sends it to
> the chrome process as a CPOW?

I assume (in fact, I'm pretty sure) that because it's a separate process, I just get the content-side version of the event listener service, which will attach the listener on the content side. So yes, the patch should, if I've not made any mistakes, just get rid of the CPOWs completely, AFAICT...


... of course, there's the initial tabbrowser.xml-induced call to _onBrowserKeypress, which in the e10s case will still lead to pretty similar CPOWs as the current case. We could probably fix this by just adding the listeners from the beginning... but IMO that should be a separate bug, because it might have sadmaking perf implications.
Comment on attachment 8576222 [details]
MozReview Request: bz://1133981/Gijs

Oh, so apparently mozreview doesn't parse reviewers out of the patch description? :-(
Attachment #8576222 - Flags: review?(mconley)
Comment on attachment 8576222 [details]
MozReview Request: bz://1133981/Gijs

https://reviewboard.mozilla.org/r/5205/#review4503

::: browser/base/content/tabbrowser.xml
(Diff revision 1)
> +                let [elt, win] = BrowserUtils.getFocusSync(this.contentDocument);

In the e10s case, it looks like gBrowser.contentDocument is undefined. I've filed bug 1144149 for that.

Even when bug 1144149 gets fixed though, getting the focus information from the contentDocument is going to cause the browser process to send down CPOW messages.

STR:

1) Install the add-on in bug 1109713 - it will click every time an unsafe CPOW usage message hits the browser console
2) Set accessibility.typeaheadfind to true (according to https://www.mozilla.org/access/type-ahead/, I need to do that in order to test FAYT).
3) Browse to some website
4) Start typing.

ER: No clicking from the add-on.

AR: Much clicking

I think we need to find some way of getting the information that this function needs via a message, OR, we need to move the processing of that information down into the content process somehow.
Attachment #8576222 - Flags: review?(mconley)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #7)
> Comment on attachment 8576222 [details]
> MozReview Request: bz://1133981/Gijs
> 
> https://reviewboard.mozilla.org/r/5205/#review4503
> 
> ::: browser/base/content/tabbrowser.xml
> (Diff revision 1)
> > +                let [elt, win] = BrowserUtils.getFocusSync(this.contentDocument);
> 
> In the e10s case, it looks like gBrowser.contentDocument is undefined. I've
> filed bug 1144149 for that.
> 
> Even when bug 1144149 gets fixed though, getting the focus information from
> the contentDocument is going to cause the browser process to send down CPOW
> messages.
> 
> STR:
> 
> 1) Install the add-on in bug 1109713 - it will click every time an unsafe
> CPOW usage message hits the browser console
> 2) Set accessibility.typeaheadfind to true (according to
> https://www.mozilla.org/access/type-ahead/, I need to do that in order to
> test FAYT).
> 3) Browse to some website
> 4) Start typing.
> 
> ER: No clicking from the add-on.
> 
> AR: Much clicking
> 
> I think we need to find some way of getting the information that this
> function needs via a message, OR, we need to move the processing of that
> information down into the content process somehow.

The tabbrowser.xml path should only be taken exactly once (because it will force the findbar to be created, which will then break that path). The other path shouldn't be producing CPOWs after these changes. Did you see where the CPOW stuff was coming from?
Flags: needinfo?(mconley)
(In reply to :Gijs Kruitbosch from comment #8)
> The tabbrowser.xml path should only be taken exactly once (because it will force the findbar to be created, which will then break that path)

Maybe this is what you meant, but it's once per tab at least since each tab creates its own findbar. Probably not the full source of the "Much clicking" though.
aEvent [1] is passed to ._onBrowserKeypress which then passes it along to ._dispatchKeypressEvent which uses its .view property [2]. Isn't that a CPOW? (still learning what exactly uses CPOWs and what doesn't, so I could be wrong, just a guess here)

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3269
[2] https://developer.mozilla.org/en-US/docs/Web/API/UIEvent/view
(In reply to Luís Miguel [:Quicksaver] from comment #10)
> aEvent [1] is passed to ._onBrowserKeypress which then passes it along to
> ._dispatchKeypressEvent which uses its .view property [2]. Isn't that a
> CPOW? (still learning what exactly uses CPOWs and what doesn't, so I could
> be wrong, just a guess here)
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#3269
> [2] https://developer.mozilla.org/en-US/docs/Web/API/UIEvent/view

The thing we pass in from browser-content is a message, not a "real" event, and it won't have a view property. See line 621 in the diff for browser-content.js
(In reply to Luís Miguel [:Quicksaver] from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> > The tabbrowser.xml path should only be taken exactly once (because it will force the findbar to be created, which will then break that path)
> 
> Maybe this is what you meant, but it's once per tab at least since each tab
> creates its own findbar. Probably not the full source of the "Much clicking"
> though.

gFindBarInitialized doesn't sound like it's once per tab...
(In reply to :Gijs Kruitbosch from comment #12)
> (In reply to Luís Miguel [:Quicksaver] from comment #9)
> > (In reply to :Gijs Kruitbosch from comment #8)
> > > The tabbrowser.xml path should only be taken exactly once (because it will force the findbar to be created, which will then break that path)
> > 
> > Maybe this is what you meant, but it's once per tab at least since each tab
> > creates its own findbar. Probably not the full source of the "Much clicking"
> > though.
> 
> gFindBarInitialized doesn't sound like it's once per tab...

... ah, but the name is a lie. Delightful. Either way, yeah, unlikely to be the source of "Much clicking".
(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to Luís Miguel [:Quicksaver] from comment #10)
> > aEvent [1] is passed to ._onBrowserKeypress which then passes it along to
> > ._dispatchKeypressEvent which uses its .view property [2]. Isn't that a
> > CPOW? (still learning what exactly uses CPOWs and what doesn't, so I could
> > be wrong, just a guess here)
> > 
> > [1]
> > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> > tabbrowser.xml#3269
> > [2] https://developer.mozilla.org/en-US/docs/Web/API/UIEvent/view
> 
> The thing we pass in from browser-content is a message, not a "real" event,
> and it won't have a view property. See line 621 in the diff for
> browser-content.js

Not that one, the one from gBrowser's own keypress handler. Originates in els.addSystemEventListener(document, "keypress", this, false) [1] -> gBrowser._handleKeyPressEvent [2] -> gFindBar._onBrowserKeypress [3]. All done in chrome script.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3269
[2] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3171
[3] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3116
(In reply to Luís Miguel [:Quicksaver] from comment #14)
> (In reply to :Gijs Kruitbosch from comment #11)
> > (In reply to Luís Miguel [:Quicksaver] from comment #10)
> > > aEvent [1] is passed to ._onBrowserKeypress which then passes it along to
> > > ._dispatchKeypressEvent which uses its .view property [2]. Isn't that a
> > > CPOW? (still learning what exactly uses CPOWs and what doesn't, so I could
> > > be wrong, just a guess here)
> > > 
> > > [1]
> > > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> > > tabbrowser.xml#3269
> > > [2] https://developer.mozilla.org/en-US/docs/Web/API/UIEvent/view
> > 
> > The thing we pass in from browser-content is a message, not a "real" event,
> > and it won't have a view property. See line 621 in the diff for
> > browser-content.js
> 
> Not that one, the one from gBrowser's own keypress handler. Originates in
> els.addSystemEventListener(document, "keypress", this, false) [1] ->
> gBrowser._handleKeyPressEvent [2] -> gFindBar._onBrowserKeypress [3]. All
> done in chrome script.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#3269
> [2]
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#3171
> [3]
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#3116

Yes, but that only happens once per tab because of gFindBarInitialized.
(In reply to :Gijs Kruitbosch from comment #15)
> Yes, but that only happens once per tab because of gFindBarInitialized.

Of course, sorry, my bad.
So strange. I can't reproduce the behaviour I saw before. Maybe I hadn't applied the patch correctly. :/

Regardless, I'm definitely not seeing unsafe CPOW usage with this patch anymore. Weird. Sorry about that!
Flags: needinfo?(mconley)
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Hey Gijs - I wasn't able to reproduce the CPOW usage I was seeing before, so I guess you're looking for another review pass from me?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #18)
> Hey Gijs - I wasn't able to reproduce the CPOW usage I was seeing before, so
> I guess you're looking for another review pass from me?

That'd be swell, although perhaps your review will boil down to "do what you suggested to do on IRC the other day" (along the lines of refactoring the browser-content listener to just always exist and getting rid of the one in tabbrowser.xml, so initial fast find keeps working) ? :-)
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8576222 - Flags: review?(mconley)
Comment on attachment 8576222 [details]
MozReview Request: bz://1133981/Gijs

https://reviewboard.mozilla.org/r/5205/#review5027

Thanks Gijs! Just a few questions and notes - see below.

::: toolkit/modules/BrowserUtils.jsm
(Diff revision 1)
> +   * if adding types to this function, please see the similar function
> +   * in browser/base/content/browser.js

I don't think this comment is relevant anymore.

::: toolkit/content/widgets/findbar.xml
(Diff revision 1)
>                return;

Should we be returning true here?

If not, it looks like everything returned from `_onBrowserKeypress` is false, so what's the case where `Findbar:Keypress`'s message sender would receive true as a result?

::: toolkit/content/widgets/findbar.xml
(Diff revision 1)
>          <parameter name="aEvent"/>

Should we rename this so that it's clear that aEvent is not actually a real DOM event, but a synthetic / serialized event?

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

We seem to be using TAF and FAYT interchangeably. Let's try to settle on one in any new code.

::: toolkit/content/browser-content.js
(Diff revision 1)
> +    addMessageListener("Findbar:UpdateFAYT", this);

This looks busted - `Findbar:UpdateFAYT` doesn't have a message listener for it.

::: toolkit/content/browser-content.js
(Diff revision 1)
> +      case "Findbar:UpdateState":
> +        this._automaticFAYT = msg.data.automaticFAYT;
> +        this._findMode = msg.data.findMode;
> +        break;

I don't see us adding a message listener for this. Should this be `Findbar:UpdateFAYT`?

::: toolkit/content/browser-content.js
(Diff revision 1)
> +        els.removeSystemEventListener(global, "keypress", this, false);
> +        els.removeSystemEventListener(global, "mouseup", this, false);

Is there any value in removing these system event listeners after they've been added? Do we avoid a leak?

::: toolkit/content/browser-content.js
(Diff revision 1)
> +

Nit - extra newline

::: toolkit/modules/BrowserUtils.jsm
(Diff revision 1)
> +   */

Can you please update the docstring with the expected parameters as well?

::: toolkit/modules/BrowserUtils.jsm
(Diff revision 1)
> +    let loc = (win || window).location;

window will not be defined in this scope. When do we expect win to be null or undefined?
Attachment #8576222 - Flags: review?(mconley)
https://reviewboard.mozilla.org/r/5205/#review5103

> Is there any value in removing these system event listeners after they've been added? Do we avoid a leak?

The findbar supports attaching to a different browser element. In that case we should stop listening in the old browser because that shouldn't influence the findbar anymore. This switching is what the Findbar:Connect and Findbar:Disconnect messages implement. It's basically the same thing findbar.xml does pre-patch.

> Should we rename this so that it's clear that aEvent is not actually a real DOM event, but a synthetic / serialized event?

Renamed to aFakeEvent, but note that tabbrowser.xml calls this with a real event. We may be able to fix that...

> Should we be returning true here?
> 
> If not, it looks like everything returned from `_onBrowserKeypress` is false, so what's the case where `Findbar:Keypress`'s message sender would receive true as a result?

Technically, the sender checks strict equality, so undefined is fine, but I've changed it to true for clarity.
Comment on attachment 8576222 [details]
MozReview Request: bz://1133981/Gijs

/r/5207 - Bug 1133981 - e10s-ify findbar FAYT key handling, r?mconley

Pull down this commit:

hg pull review -r 94ba154e69f587eec6692f95ad806761040b24da
Comment on attachment 8576222 [details]
MozReview Request: bz://1133981/Gijs

(PS: I closed the issues that I believed I addressed as "fixed"... it's not clear to me if that's the intention, or if I should leave that to the reviewer... but there we are)
Attachment #8576222 - Flags: review?(mconley)
Also, be aware of felipe's patch in bug 1079665 - is there anything in there that you need to integrate into your patch to make sure you don't re-open his bug?
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #25)
> Also, be aware of felipe's patch in bug 1079665 - is there anything in there
> that you need to integrate into your patch to make sure you don't re-open
> his bug?

I don't think so, it seems like the e10s-ification of the focus handling happened there and is already using messages, so it should Just Work, I think? I guess some of this stuff is now less sync than it may have been before, so perhaps that'll make a difference. Which reminds me... I'm not sure I actually needed the test change in the end, but that would be simple to check and revert. OK, it seems I don't, new mozreview coming up.
Attachment #8576222 - Flags: review?(mconley)
Comment on attachment 8576222 [details]
MozReview Request: bz://1133981/Gijs

/r/5207 - Bug 1133981 - e10s-ify findbar FAYT key handling, r?mconley

Pull down this commit:

hg pull review -r 76f3cf7de4ab3c60c0656c45c36933c626c7a967
Attachment #8576222 - Flags: review?(mconley)
Comment on attachment 8576222 [details]
MozReview Request: bz://1133981/Gijs

https://reviewboard.mozilla.org/r/5205/#review5165

I think this looks good to go, from inspection. Just one last thing.

::: toolkit/content/widgets/findbar.xml
(Diff revisions 1 - 3)
>  

Alright, so we sometimes get a fake event and sometimes get a real one. *sigh*

At the very least, we should document that here. And we should also document what the return values mean to the callers.
Attachment #8576222 - Flags: review?(mconley) → review+
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
All the things I did with the latest revision:

- rebased, which seems to throw off the interdiff functionality in reviewboard... sorry!

- always listen for keypresses in the content script
- fetch the pref value when this is hit, in the same way that the tabbrowser.xml code used to. I considered using an observer but it seems that this script is created once per <browser> so that seemed like serious overkill.
- Removed the tabbrowser.xml keypress event forwarding
- Made the tabbrowser keypress *event* listener OSX-only so as to not listen in two places on most platforms (I expect there are $reasons we have the listener on OSX rather than just using a <key> XUL element for what it does (tab switching)...)
- Made tabbrowser.xml listen for the keypress messages and force creation of the findbar using those instead of CPOW-filled events

- made use of the fancy new member() {} syntactic sugar for object method/functions, like the printing object
- Renamed _automaticFAYT to _useTypeAheadFind and unified naming between findbar and browser-content.js
- ensured we don't stick functions on the fake event because they make the message nonclonable (was showing warnings in the browser console)
- correctly process the result of sendSyncMessage (it returns an array)
- added docs for the fakeEvent stuff

- Added Services.jsm getter for the system event listener service because we use it all over the place and I'm getting bored of reading all the Cc[blashblafshg] stuff.
Comment on attachment 8576222 [details]
MozReview Request: bz://1133981/Gijs

/r/5207 - Bug 1133981 - e10s-ify findbar FAYT key handling, r?mconley

Pull down this commit:

hg pull -r 31f07c93b1f5116a5bb952b1c19da003941ad762 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8576222 - Flags: review+ → review?(mconley)
Comment on attachment 8576222 [details]
MozReview Request: bz://1133981/Gijs

https://reviewboard.mozilla.org/r/5205/#review5749

This looks good to me. Just a few questions / issues. When addressed, ship away!

::: toolkit/modules/Services.jsm
(Diff revision 4)
> +  ["els", "@mozilla.org/eventlistenerservice;1", "nsIEventListenerService"],

We should update the Services.jsm documentation to reflect this addition.

::: toolkit/content/browser-content.js
(Diff revision 4)
> -    Cc["@mozilla.org/eventlistenerservice;1"]
> +    Services.els.addSystemEventListener(global, "mousedown", this, true);

Ah, much better. Thanks. :)

::: toolkit/content/browser-content.js
(Diff revision 4)
> +    var rv = sendSyncMessage("Findbar:Keypress", fakeEvent);

let, not var

::: toolkit/content/browser-content.js
(Diff revision 4)
> +    if (rv.indexOf(false) !== -1) {

indexOf? Why is rv an array? Looking at the handler in the parent, it should just return true or false, no?
Attachment #8576222 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #31)
> Comment on attachment 8576222 [details]
> MozReview Request: bz://1133981/Gijs
> 
> https://reviewboard.mozilla.org/r/5205/#review5749
> 
> This looks good to me. Just a few questions / issues. When addressed, ship
> away!
> 
> ::: toolkit/modules/Services.jsm
> (Diff revision 4)
> > +  ["els", "@mozilla.org/eventlistenerservice;1", "nsIEventListenerService"],
> 
> We should update the Services.jsm documentation to reflect this addition.

Will do.

> ::: toolkit/content/browser-content.js
> (Diff revision 4)
> > -    Cc["@mozilla.org/eventlistenerservice;1"]
> > +    Services.els.addSystemEventListener(global, "mousedown", this, true);
> 
> Ah, much better. Thanks. :)
> 
> ::: toolkit/content/browser-content.js
> (Diff revision 4)
> > +    var rv = sendSyncMessage("Findbar:Keypress", fakeEvent);
> 
> let, not var

Done.

> ::: toolkit/content/browser-content.js
> (Diff revision 4)
> > +    if (rv.indexOf(false) !== -1) {
> 
> indexOf? Why is rv an array? Looking at the handler in the parent, it should
> just return true or false, no?

Yes, but sendSyncMessage will consolidate all the replies into an array, according to the docs at:

https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/The_message_manager#Synchronous_messaging

Specifically, it says:

> Because a single message can be received by more than one listener, the return value of sendSyncMessage() is an array of all the values returned from every listener, even if it only contains a single value.


remote:   https://hg.mozilla.org/integration/fx-team/rev/6bd63b661b78
Keywords: dev-doc-needed
Whiteboard: [fixed-in-fx-team]
(In reply to :Gijs Kruitbosch from comment #32)
> > ::: toolkit/content/browser-content.js
> > (Diff revision 4)
> > > +    if (rv.indexOf(false) !== -1) {
> > 
> > indexOf? Why is rv an array? Looking at the handler in the parent, it should
> > just return true or false, no?
> 
> Yes, but sendSyncMessage will consolidate all the replies into an array,
> according to the docs at:
> 
> https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/
> The_message_manager#Synchronous_messaging
> 
> Specifically, it says:
> 
> > Because a single message can be received by more than one listener, the return value of sendSyncMessage() is an array of all the values returned from every listener, even if it only contains a single value.
> 

Ah, gotcha. Thanks for the explanation!
Updated the MDN page for Services.jsm .
Keywords: dev-doc-needed → dev-doc-complete
https://hg.mozilla.org/mozilla-central/rev/6bd63b661b78
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla40
Depends on: 1154799
Reproduced this issue on Windows 7 64-bit and Mac OS X 10.9.5 with 2015-02-17 Nightly (build ID: 20150217030213).

The error is no longer thrown in the console using latest Nightly, build ID: 20150419030206.
Verified on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 12.04 32-bit.
Status: RESOLVED → VERIFIED
status-firefox40: fixed → verified
QA Contact: cornel.ionce

        Attachment #8576222 -
        Attachment is obsolete: true

        Attachment #8619500 -
        Flags: review+

    
    

    
  
Aargh, you moved important parts of findbar out of toolkit, thus breaking it for everyone else...

    
    

    
  
(In reply to neil@parkwaycc.co.uk from comment #40)
> Aargh, you moved important parts of findbar out of toolkit, thus breaking it
> for everyone else...

Sorry, you confused me by moving it to a file called browser-content.js. Now I have to find out why it doesn't seem to be loading for me.

    
    

    
  
OK, I see what you've done, the findbar now only works with an actual <browser> element, whereas before it would work with a <tabbrowser> element too.

    
    

    
  
Eww, you're copying all the inherited properties. You probably want to use Object.keys(event) or event.hasOwnProperty(k).

    
    

    
  
(In reply to neil@parkwaycc.co.uk from comment #43)
> Eww, you're copying all the inherited properties. You probably want to use
> Object.keys(event) or event.hasOwnProperty(k).

I got curious and checked this out. Both those methods only get a single property out of the keypress event: 'isTrusted'. That makes sense because the event object inherits from KeyboardEvent [1], so all the properties the findbar wants are inherited after all.

Also, even if it copied only the properties inherited directly from KeyboardEvent, it would still need .bubbles, .cancelable, and .type [2], which come from Event [3] (.view even comes from UIEvent [4], but that's an object so it's already not passed at all).

The alternative would be to hardcode which properties (or values?) to send, but that's probably more future-error-prone if the code is changed without taking into consideration this is a fakeEvent and not an actual event.

I liked this, I learned. :)

[1] https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent
[2] http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#655
[3] https://developer.mozilla.org/en-US/docs/Web/API/Event
[4] https://developer.mozilla.org/en-US/docs/Web/API/UIEvent

    
    

    
  
(In reply to Luís Miguel from comment #44)
> (In reply to comment #43)
> > Eww, you're copying all the inherited properties. You probably want to use
> > Object.keys(event) or event.hasOwnProperty(k).
> 
> I got curious and checked this out. Both those methods only get a single
> property out of the keypress event: 'isTrusted'. That makes sense because
> the event object inherits from KeyboardEvent [1], so all the properties the
> findbar wants are inherited after all.
> 
> Also, even if it copied only the properties inherited directly from
> KeyboardEvent, it would still need .bubbles, .cancelable, and .type [2],
> which come from Event [3] (.view even comes from UIEvent [4], but that's an
> object so it's already not passed at all).
> 
> The alternative would be to hardcode which properties (or values?) to send,
> but that's probably more future-error-prone if the code is changed without
> taking into consideration this is a fakeEvent and not an actual event.

My mistake was to confuse inherited properties with properties of the constructor. Out of the 243 properties of a KeyboardEvent, 199 are duplicates of properties of KeyboardEvent and 8 of Event, so only 25 KeyboardEvent instance properties and 19 Event instance properties are actually interesting.

    
    

    
  
Pff, I go away and I miss all the fun. Neil, can you clarify in a followup bug if there's anything that needs to be addressed here?
Flags: needinfo?(neil)

    
    

    
  
(In reply to Gijs Kruitbosch from comment #46)
> Pff, I go away and I miss all the fun. Neil, can you clarify in a followup
> bug if there's anything that needs to be addressed here?

I guess I can file a bug on tidying that up...
Flags: needinfo?(neil)
Blocks: 1190382

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: