1211296 - Investigate loading very small images by default with tap-to-load images enabled

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

I'm not sure exactly what's causing this, but logging into the American Express rewards website fails if tap-to-load images are enabled.

Comment 1

[Mike Taylor [:miketaylr]]

It's hard to diagnose too much w/o a test account, but I wonder if we're not blocking some "tracking pixel" image that somehow the site depends on.

Testing with fake creds, w/ tap-to-load images disabled I see 2 similar requests sending a ton of junk as GET params:

> https://omns.americanexpress.com/b/ss/amexpressserprod/1/JS-1.3.2/s9465068574722?AQB=1&ndh=1&t=5/9/2015 10:18:41 1 300&fid=4A791C84D416AFC3-280417CA0D0ADB0A&ce=UTF-8&ns=1americanexpress&fpCookieDomainPeriods=2&pageName=US|AMEX|Ser|EnterpriseLogin&g=https://online.americanexpress.com/myca/logon/us/action/loglogonhandler?request_type=loglogonhandler&location=us_logon1&r=https://online.americanexpress.com/myca/logon/us/action/LogonHandler?request_type=LogonHandler&Face=en_US&inav=iNFootLgBtn&cc=USD&pe=lnk_o&pev2=US:Rich Media Action&c3=en&c26=US:Ser:Enterpriselogin:InValidUserIDorPwd&c49=Ser r18.0.2&s=360x640&c=24&j=1.8.5&v=N&k=Y&bw=360&bh=567&AQE=1

With tap-to-load enabled, that request is blocked. Weak hunch, but hard to do much more w/o an account.

Comment 2

[:Margaret Leibovic]

I'll look into this, since I have the account to reproduce.

Comment 3

[:Margaret Leibovic]

I've been looking into this, but I'm not sure how to figure out exactly what's going wrong.

With tap-to-load images disabled, I see a POST request with my login info, but when the images are blocked, I only see a GET. Maybe there's some logic earlier somewhere that determines it doesn't have enough info to do a proper POST and just falls back to a GET for an error page?

Assuming it is some sort of tracking pixel, I'm going to try to see if I can find the image that needs to be loaded in order to make this work. But even if that's the case, I don't know what a patch to fix this would look like... if it is a 1x1 pixel, maybe we should just always load all tiny images like that.

Comment 4

[:Margaret Leibovic]

Jonathan, can you help me figure out how to debug this? Is there a way to throw together a patch that will let through "small" images? Or a patch that logs the URLs that are being blocked, such that I can optionally choose to load some but not others?

Comment 5

[Jonathan Almeida [:jonalmeida]]

This bug got past me without seeing it!

We can do this by either looking up the image dimensions or the file size.

To get the dimensions, it should be as easy as adding this:
```
var img = new Image();
img.src = imgSrc;
if (img.height * img.width <= 64*64) { // images with only 4k pixels
   return Ci.nsIContentPolicy.ACCEPT;
}
```

In my bug 1208167 review, I've included an option to make a HEAD request for the image and use the 'Content-Length' to roughly determine the file size of the image. This is the bit of code in the review that does the magic:
```
function sendImageSizeTelemetry(imgSrc) {
  let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
  xhr.open("HEAD", imgSrc, true);
  xhr.onreadystatechange = function (e) {
    if (xhr.readyState != 4) {
        return;
    }
    if (xhr.status != 200) {
        return;
    }
    let contentLength = xhr.getResponseHeader("Content-Length");
    let imageSize = contentLength / 1024;
    Services.telemetry.getHistogramById(TELEMETRY_SHOW_IMAGE_SIZE).add(imageSize);
  };
  xhr.send(null);
}
```

I'm not entirely sure of what overhead comes with the former method since don't know how it works under the hood (i.e. when we set the src attribute, how does it get the image dimensions if it hasn't downloaded the image yet? Does it also send a HEAD request or some other fancy way?

Comment 6

[:Margaret Leibovic]

I haven't had time to get back to this bug. Jonathan, would you be interested in making a speculative patch here (with a bunch of logging), and I can see how it handles this Amex testcase?

Comment 7

[Jonathan Almeida [:jonalmeida]]

Yes! I'll be working on this now that I've landed my other bugs which make it easier to fix this.

Will send you a patch to test once I something working.

Comment 8

[Jonathan Almeida [:jonalmeida]]

After chatting with some of the folks in the necko channel, they recommended to continue making HEAD requests or making a range request[1] for the first few bytes, which is where the file size of the image would be at. The latter seems more complicated at the moment.

I've started testing out a patch that I put together, but it doesn't seem to be working as expected. Will try it again; currently tight with time.

FWIW, creating an image object and setting the src url automatically downloads the image which is how it gets the image dimensions.

[1]: https://greenbytes.de/tech/webdav/draft-ietf-httpbis-p5-range-latest.html

Comment 9

[Jonathan Almeida [:jonalmeida]]

Attachment: Allow very small images (tracking pixels) to be downloaded

Comment 10

[Jonathan Almeida [:jonalmeida]]

Sorry about going MIA for a while! I've put together this crude patch which seems to work for me in my tests. For now, I'm allowing images with size under 4kB to be downloaded.

Margaret, could you try out the above patch with your test page to see if it works?

I've made a debug apk available here, but I recommend applying the patch in your local build and using that instead (since the test page is a credit card website :).

Debug apk: http://fennec.surge.sh/app-allow-small-images-bug-1211296.apk

/me polishes tinfoil hat.

Comment 11

[Jonathan Almeida [:jonalmeida]]

Could you also see if bug 1220142 is still reproducible with this patch? I'm able to see the center image now with this build as well.

Comment 12

[Jonathan Almeida [:jonalmeida]]

Attachment: Allow very small images (tracking pixels) to be downloaded

Comment 13

[Jonathan Almeida [:jonalmeida]]

Cleaned up my previous patch; still functions as normal.

Comment 14

[:Margaret Leibovic]

Unfortunately I can't reproduce this problem anymore on Nightly, so I can't verify whether this patch fixes it. I'm not sure it's worth going ahead and landing this patch unless we hear of other sites that have a similar problem.

That being said, the "Log In" button isn't visible when tap-to-load images are enabled:
https://www.dropbox.com/s/35z6qpy6abd6chl/2015-12-07%2020.28.10.png?dl=0
https://www.dropbox.com/s/813p2di6yuvm2bn/2015-12-07%2020.27.34.png?dl=0 

But that seems like more of a website issue, not something we can solve by loading small images.

Comment 15

[Jonathan Almeida [:jonalmeida]]

Attachment: preview.png

I noticed I can't repro the issue with the Medium bug either on the latest nightly.

This patch does fix the aesthetic looks for some pages though.

The screenshots on the left are using my patch; right is nightly.

Comment 16

[:Margaret Leibovic]

(In reply to Jonathan Almeida (:jonalmeida) from comment #15)
> Created attachment 8696766 [details]
> preview.png
> 
> I noticed I can't repro the issue with the Medium bug either on the latest
> nightly.
> 
> This patch does fix the aesthetic looks for some pages though.
> 
> The screenshots on the left are using my patch; right is nightly.

I don't see a difference here... can you point out what you think looks better?

Comment 17

[Mike Taylor [:miketaylr]]

(In reply to :Margaret Leibovic from comment #16)
> I don't see a difference here... can you point out what you think looks
> better?

On the left, we have a Google-menu-burger and the DDG logo are now visible. Looks better!

Comment 18

[:Margaret Leibovic]

(In reply to Mike Taylor [:miketaylr] from comment #17)
> (In reply to :Margaret Leibovic from comment #16)
> > I don't see a difference here... can you point out what you think looks
> > better?
> 
> On the left, we have a Google-menu-burger and the DDG logo are now visible.
> Looks better!

Ah, I see now :)

Maybe we should go ahead and land this, then. Do we have a way to measure how much this impacts data usage? If it's not a big increase, this could be a nice way to make this feature more usable.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8696311 [details] [diff] [review]
Allow very small images (tracking pixels) to be downloaded


>diff --git a/mobile/android/components/ImageBlockingPolicy.js b/mobile/android/components/ImageBlockingPolicy.js

>@@ -48,38 +56,48 @@ ImageBlockingPolicy.prototype = {

>         if (node instanceof Ci.nsIDOMHTMLImageElement) {
>+          this._imageSize(contentLocation.spec, function(imgSize) {
>+            // Allowing images that are under 3kB in size which is a bit arbitrary for now
>+            if (imgSize <= 3072) {

I'd like to see this number in a pref, "browser.image_blocking.allowSize", in bytes where 0 = "skip this check and just block"


>+  _imageSize: function(imageURL, callback) {
>+    let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
>+    xhr.open("HEAD", imageURL, true);
>+    xhr.onreadystatechange = function (e) {
>+      if (xhr.readyState != 4) {
>+        return;

We should probably call the callback with an unknown size (maybe -1 or maybe a HUGE #)

>+      }
>+      if (xhr.status != 200) {
>+        return;

We should probably call the callback with an unknown size (maybe -1 or maybe a HUGE #)

>+      }
>+      let contentLength = xhr.getResponseHeader("Content-Length");
>+      if (!contentLength) {
>+        return;

We should probably call the callback with an unknown size (maybe -1 or maybe a HUGE #)

>+      if (callback) {
>+        callback(contentLength)
>+      } else {
>+        return contentLength;
>+      }

Do we care about the non-callback case? I don't think we should.

Comment 20

[Jonathan Almeida [:jonalmeida]]

(In reply to :Margaret Leibovic from comment #18)
> (In reply to Mike Taylor [:miketaylr] from comment #17)
> > (In reply to :Margaret Leibovic from comment #16)
> > > I don't see a difference here... can you point out what you think looks
> > > better?
> > 
> > On the left, we have a Google-menu-burger and the DDG logo are now visible.
> > Looks better!
> 
> Ah, I see now :)
> 
> Maybe we should go ahead and land this, then. Do we have a way to measure
> how much this impacts data usage? If it's not a big increase, this could be
> a nice way to make this feature more usable.

I asked the necko guys about this (comment 8), and making HEAD requests is the cheapest way in terms of network usage we can provide this feature. Now, we make a request as normal to every image, but we just don't download them. Without the patch, when we make see an image type it straight up gets blocking from having that request go through.

TL;DR: Same number of network requests as without image blocking, but without downloading the full image.

Comment 21

[Jonathan Almeida [:jonalmeida]]

(In reply to Mark Finkle (:mfinkle) from comment #19)
> Comment on attachment 8696311 [details] [diff] [review]
> Allow very small images (tracking pixels) to be downloaded
> 
> 
> >diff --git a/mobile/android/components/ImageBlockingPolicy.js b/mobile/android/components/ImageBlockingPolicy.js
> 
> >+      if (callback) {
> >+        callback(contentLength)
> >+      } else {
> >+        return contentLength;
> >+      }
> 
> Do we care about the non-callback case? I don't think we should.

Ah, I was experimenting with returning the value for telemetry. Will remove that bit.

Comment 22

[Jonathan Almeida [:jonalmeida]]

Unassigning. I'm not actively working on this.

Comment 23

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.