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

RESOLVED FIXED in Firefox 35

Status

()

Firefox
Extension Compatibility
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: iamjayakumars, Assigned: billm)

Tracking

({addon-compat})

35 Branch
Firefox 35
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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
tracking-e10s: --- → +
Summary: "Google/Yandex search link fix" add-on does not work with e10s → "Google/Yandex search link fix" add-on does not rewrite SERP's redirect links with e10s enabled
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?
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.
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 3

3 years ago
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.
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 4

3 years ago
Created attachment 8481657 [details] [diff] [review]
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.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8481657 - Flags: review?(mconley)
(Assignee)

Comment 5

3 years ago
Created attachment 8481661 [details] [diff] [review]
event-fix

Forgot the test.
Attachment #8481657 - Attachment is obsolete: true
Attachment #8481657 - Flags: review?(mconley)
Attachment #8481661 - Flags: review?(mconley)
(Assignee)

Comment 6

3 years ago
er, the comment
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?
(Assignee)

Comment 8

3 years ago
(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.
Attachment #8481661 - Flags: review?(mconley) → review+
OS: Mac OS X → All
Hardware: x86 → All
Version: 34 Branch → 35 Branch
(Assignee)

Comment 9

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ab4be446c67
https://hg.mozilla.org/mozilla-central/rev/1ab4be446c67
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35

Updated

3 years ago
Depends on: 1097532
You need to log in before you can comment on or make changes to this bug.