1113967 - adblock plus does not hide the elements any more when turn on e10s

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[[e-mail]]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141219090545

Steps to reproduce:

element hiding does not work any more. no rule of hiding appears in the blockable item list.
bad http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1418953031/
good http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1418950322/

Comment 1

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

I might have regressed this in bug 1072980.

Comment 2

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

Attachment: fix-adblock

It turns out that setting notificationCallbacks to null breaks adblock. It uses notificationCallbacks to get the window associated with a given request.

The reason for removing notificationCallbacks was bug 1072980 comment 27. I hacked around the CPOW issue by creating a normal JS object that wraps the CPOW.

Comment 3

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

Attachment: adblock-test

This test more closely mimics what adblock does so that we don't regress this again.

Comment 4

[Allison Naaktgeboren :ally]

Comment on attachment 8540475 [details] [diff] [review]
fix-adblock

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

Since I approved the patch that caused the regression, I think it would be wise to have a different set of informed eyes check the overall architecture of these callbacks & their uses, lest we be creating more bugs for ourselves.

The hack, as hacks go, looks pretty clean.

Comment 5

[Allison Naaktgeboren :ally]

Comment on attachment 8540476 [details] [diff] [review]
adblock-test

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

Since I don't feel qualified to fully review the change, I probably shouldn't approve the test out of hand. Assuming the patch is dandy, I think this looks pretty good.

::: toolkit/components/addoncompat/tests/addon/bootstrap.js
@@ +413,5 @@
>          request.send(null);
>          if (request.status != 200) {
>            throw(`about:test1 response had status ${request.status} - expected 200`);
>          }
> +        if (request.responseText.indexOf("test1") == -1) {

What exactly does '-1' mean in this context? Might we add a comment?

@@ +463,5 @@
> +    // This needs to be a chrome-privileged page that loads in the
> +    // content process. It needs chrome privs because otherwise the
> +    // XHRs for about:test[12] will fail with a privilege error
> +    // despite the presence of URI_SAFE_FOR_UNTRUSTED_CONTENT.
> +    let newTab = gBrowser.addTab("chrome://addonshim1/content/page.html");

oof. Thanks for the comment.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8540475 [details] [diff] [review]
fix-adblock

Took me a little while to figure out how this worked (haven't looked at the shims closely before), but this makes sense.

The reasoning in bug 1072980 comment 27 (leading to "we might as well just set it to null") seems dangerous to me. I appreciate that we're in hacky territory with all of this stuff to begin with, but it seems like investing in a bit more effort up front to make shim behavior be consistent (and more completely replicate the behavior of the underlying APIs) is likely to pay off by avoiding subtle bugs/compat issues down the road.

Comment 7

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

Comment on attachment 8540476 [details] [diff] [review]
adblock-test

Could you please look at this Mike?

Comment 8

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

Comment on attachment 8540476 [details] [diff] [review]
adblock-test

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

This looks good - just a minor point to follow-up what ally spotted.

Thanks for this, Bill!

::: toolkit/components/addoncompat/tests/addon/bootstrap.js
@@ +413,5 @@
>          request.send(null);
>          if (request.status != 200) {
>            throw(`about:test1 response had status ${request.status} - expected 200`);
>          }
> +        if (request.responseText.indexOf("test1") == -1) {

ally:

The createAndRegisterAboutModule function I wrote creates an about:SOMEID page for testing, and makes that about: page's content be:

data:,<html><h1>${SOMEID}</h1></html>

(this patch moves the logic that does that into TestChannel - see line 299)

So the check here is making sure that the about: page has loaded the right content that createAndRegisterAboutModule is supposed to be supplying at that address.

Yeah, a comment here saying that we're checking that the content matches the ID of the page sounds like a good idea.

@@ +424,5 @@
>  
>          if (request.status != 200) {
>            throw(`about:test2 response had status ${request.status} - expected 200`);
>          }
> +        if (request.responseText.indexOf("test2") == -1) {

Probably it'd be more readable to do:

if (!request.responseText.contains("test1")) {
  // ...
}

same above at the point that ally spotted.

Comment 9

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

These patches landed in bug 1072980.