1059207 - "Google/Yandex search link fix" add-on does not rewrite SERP's redirect links with e10s enabled

Status: RESOLVED FIXED

(Firefox :: Extension Compatibility, defect)

Description

[iamjayakumars]

I can able to see the direct link in google, if I copy that link, then its mix of google and next time it shows the mix of google link instead of a direct link.

https://addons.mozilla.org/en-US/firefox/addon/google-search-link-fix/

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140826030203 CSet: dc352a7bf234

Comment 1

[Wladimir Palant]

The problem is, the extension needs to recognize Google's search pages somehow. For that it uses a sandbox to access the window.google variable:

>  let sandbox = new Cu.Sandbox(window);
>  sandbox.window = window;
>  return Cu.evalInSandbox("(function(){var g = window.wrappedJSObject.google; return g && (g.sn || g.search) ? true : false;})()", sandbox);

However, with e10s window.wrappedJSObject.google is always undefined - regardless of whether a sandbox is used. Same thing with XPCNativeWrapper.unwrap(window).google. Is there a simple way to access variables from a content page, without using frame scripts?

Comment 2

[Chris Peterson [:cpeterson]]

Bill: Wladimir would like to know (^ comment 1) if there is a simple way for the parent process to read a content page's variables without frame scripts.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Well, using a frame script would be the best way to do this. In fact, it seems like this add-on would be a great candidate for Jetpack.

However, we'd also like to make it work in e10s unchanged. I tested things with the fix for bug 1060046 applied and it still didn't work. The problem seems to be with how the mouse events are passed up from the child. I'll post a patch for that.

I didn't see anything wrong with the sandbox code. We have special shims to run sandbox code in the child process and they seem to be working in this case. Maybe the problem was caused by bug 1060046.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: event-fix

The add-on does some tricky stuff where it adds two mousedown listeners--one with useCapture=true and one with useCapture=false. I mainly looked at the first listener (useCapture=true); let's call it L.

In normal Firefox, L is called once per click. In e10s, I saw it called three times. The first time, it was called with the entire <xul:browser> element as the target; that event was dispatched in the parent, since all it knows about is the <xul:browser> element. The second and third times were sent by the child--one during capturing and one during bubbling. L is only supposed to be called during capturing, but the parent code doesn't distinguish right now, so we were calling it in both cases.

To eliminate the first call to L, I write code to ignore events dispatched by the parent to remote <xul:browser> elements. To eliminate the third call, I added code to distinguish the useCapture cases.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: event-fix

Forgot the test.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

er, the comment

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8481661 [details] [diff] [review]
event-fix

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

::: toolkit/components/addoncompat/RemoteAddonsParent.jsm
@@ +492,5 @@
> +  if (filteringListeners.has(listener)) {
> +    return filteringListeners.get(listener);
> +  }
> +
> +  function filter(event) {

If the filter has a closure with the listener, doesn't this prevent the listener from being GC'd, and prevent the WeakMap from automatically depopulating? Like, haven't we created a circular reference, whereby the weakly mapped key has a reference held to it in the value?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #7)
> If the filter has a closure with the listener, doesn't this prevent the
> listener from being GC'd, and prevent the WeakMap from automatically
> depopulating? Like, haven't we created a circular reference, whereby the
> weakly mapped key has a reference held to it in the value?

That's fine. The GC is smart enough to collect the cycle. We'll only mark the value (filter) if the key (listener) has been marked. So filter can't cause listener to be marked.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ab4be446c67

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1ab4be446c67