1349921 - Cached iframe executes previously loaded and dynamically inserted scripts, makes network calls before "onload" event.

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[jurij.sinickij]

Attachment: Test page, network sessions.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce:

1. Serve attached files on web server (cache headers needed for frame.html)
2. Use proxy to track network calls (dev tools shows that everything works well)
3. Open index.html via web server URL
4. Refresh couple times, back-forward navigation forces bug appearing
5. Track network calls until you will notice unnecessary calls from the past.



Actual results:

Not needed network calls are made. This means that browser somewhy executes stored in cache scripts.



Expected results:

Only defined actions and network calls should be made.

Network sessions attached.

Comment 1

[:Gijs (he/him)]

Not a security issue.

Comment 2

[jurij.sinickij]

Could you prioritise this issue as it highly impacts whole AdTech industry? There is a huge number of fake ad impressions generated due to this bug as almost all ad placements are iframes.

Comment 3

[:Gijs (he/him)]

(In reply to jurij.sinickij from comment #2)
> Could you prioritise this issue as it highly impacts whole AdTech industry?
> There is a huge number of fake ad impressions generated due to this bug

Do you have evidence for these statements?

TBH, all I did was some initial triage to get this out of the security group where nobody would see it, as it clearly wasn't a security bug (nor a Firefox UI bug, hence moving it to Core). More triage will be done at some point in the future. I tried to look into this a bit to evaluate your assertion that this is "high impact", but actually:

0) your har files don't seem to be readable on http://www.softwareishard.com/har/viewer/
1) your iframe includes remote script that's probably minified (they're adscripts, after all), making it hard to understand what the problem is
2) your description of "unnecessary calls from the past" is unclear. What do you actually mean? What specific requests (or calls? script calls? something else?) are problematic, and where are they coming from? Is a proxy actually required here or are those requests to localhost that will just show up in any ordinary server log? Why do you believe those requests shouldn't be made?
3) it's not clear to me there's any bug here. The code in the iframe uses document.write after the document has loaded, thus creating another document (and history entry). If you refresh a "couple of times", that will likely create a few such disembodied documents. That requests can be made from those documents (up to the point where they are completely unloaded) doesn't seem like a bug to me.

To triage this reasonably we will need more details from you.

Comment 4

[jurij.sinickij]

Attachment: Bug with image content.

I am working for AdTech company and after Firefox 52 we noticed increase (about 5x) in ad requests from Firefox browser, but actual number of ads our scripts are able to track left the same.
All the billing in internet advertising is based on ad requests number this the reason I have asked for priority.

0. probably 'charles' generates har files incorrectly. Anyway, request logs could tell nothing about this bug.
1. sorry for bad example, updated test page with simple image and timeout, seems bug is not related to script execution it's more like weird cache behaviour.
2. here you can find videos with bug reproducing to get the thing what actually goes wrong and why I thing it's related to cache.
Cache disabled - works as it should, makes only one call to fetch an image.
https://transfer.sh/IhhZo/good.mov
Cache enabled - takes some time to reproduce, while loading cached iframe browser requests previously loaded images (render is not impacted) which are not relevant for current execution.
https://transfer.sh/uaEib/bad.mov

Probably bug could be related to this one: https://bugzilla.mozilla.org/show_bug.cgi?id=1349006

Comment 5

[:Gijs (he/him)]

(In reply to jurij.sinickij from comment #4)
> Created attachment 8850599 [details]
> Bug with image content.
> 
> I am working for AdTech company and after Firefox 52 we noticed increase
> (about 5x) in ad requests from Firefox browser, but actual number of ads our
> scripts are able to track left the same.

So are you saying this is a regression compared to 51? That information wasn't in comment #0...

If this is a regression, can you reproduce with Firefox Nightly? ( https://nightly.mozilla.org/ )

If so, can you try to find a more specific regression window with the dedicated tool we have for this? ( https://mozilla.github.io/mozregression/ )

A regression window will help figure out what broke this.

Boris, flagging you here as you reopened bug 1349006 so maybe you can make something out of the har files and/or the new testcase (or know someone who can).

Comment 6

[Boris Zbarsky [:bzbarsky]]

ccing some people who are looking into something like this already.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Just to make sure, which cache is being referred to in the "cache enabled"/"cache disabled" bit in comment 4?  How exactly is it being enabled/disabled?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Bug 1349006 doesn't seem related, since it reproduces in 51 as well.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I did change window frozen/suspend states in FF52.  Could this be fallout from bug 1303167?

Comment 10

[Boris Zbarsky [:bzbarsky]]

And in particular, bug 1349006 has to do with a subframe which varies response based on referrer but doesn't send the corresponding Vary header.  As far as I can tell, that bug report is invalid.  Still nothing to do with this bug.  ;)

Comment 11

[jurij.sinickij]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #7)
> Just to make sure, which cache is being referred to in the "cache
> enabled"/"cache disabled" bit in comment 4?  How exactly is it being
> enabled/disabled?

Disabled means - cache size is set to 0 MB.

Comment 12

[jurij.sinickij]

(In reply to :Gijs from comment #5)

> So are you saying this is a regression compared to 51? That information
> wasn't in comment #0...
> 
> If this is a regression, can you reproduce with Firefox Nightly? (
> https://nightly.mozilla.org/ )

I can confim that it is a regression compared to FF 51.0.1, bug also appears in FF nightly.

Will look for regression window.

Comment 13

[jurij.sinickij]

Regression testing result.

Fist bad version - b3b5be1d2027f915c3c54b966fb41de8444923ee
Last good version - a603640aa82d54b224b2325d8a445fd14dd00bc6
Push log - https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a603640aa82d54b224b2325d8a445fd14dd00bc6&tochange=b3b5be1d2027f915c3c54b966fb41de8444923ee

Seems that bug introduced after bug 1016628

Comment 14

[Boris Zbarsky [:bzbarsky]]

Hmm.  That says fixed for 48, but presumably it got disabled there, and based on bug 1278636 also disabled in 49.  Sadly, the firefox-49 flag wasn't set to "disabled", and now it's not visible.  :(

Nick, do you know what release we actually shipped that in?  Did it ship in 50, or later?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Poking around on the old branches, looks like on FIREFOX_51_0_1_RELEASE we have:

  #ifdef NIGHTLY_BUILD
  pref("network.predictor.enable-prefetch", true);
  #else
  pref("network.predictor.enable-prefetch", false);
  #endif

so predictor prefetch was disabled on 51.  Same thing on 50.  On 52, it's enabled by default, looks like.  So very plausibly this is the thing that people are hitting.  I wish the tracking information on bug 1278636 were correct.  :(

Comment 16

[Boris Zbarsky [:bzbarsky]]

Bug 1304387 is what enabled the predictor prefetch on 52.

Comment 17

[Boris Zbarsky [:bzbarsky]]

jurij, thank you for hunting that down!

Comment 18

[jurij.sinickij]

FYI, bug 1350032 talks totally about the same thing as this one.

Comment 19

[u408661]

Seems pretty clear prefetch is causing this. So for the short-term fix, we should probably just push out a hotfix to flip the pref to false, disabling prefetch. Longer-term, I think the fix is at least one, if not both of: (1) don't prefetch things with query strings, and (2) ensure that prefetch is only kicked off for explicit user navigations (as opposed to session restore/similar, as well).

Comment 20

[u408661]

Attachment: hotfix patch

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8851743 [details] [diff] [review]
hotfix patch

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

Looks ok, but we have deprecated hotfixes in favor of system addons. The code will be very similar but the release process is a bit different. The repo to be used is now https://github.com/mozilla/one-off-system-add-ons

Take a look at this example which is the simplest one: https://github.com/mozilla/one-off-system-add-ons/tree/master/addons/hsts-priming

And one comment about the code: If you set the pref this way, all users with existing profiles will have stored network.predictor.enable-prefetch to be false forever. It's fine but you'll have to change the pref name in newer versions so that they get the default value of "true" later on when the problem is solved and prefetch can be re-enabled again.

Or you can do it the same way as the hsts-priming addon did and set it through the defaultPrefs, which don't stick after a restart. It means your system addon will keep running during the entire duration of 52, but it's fine since it's invisible. And once it is removed (automatically when Firefox updates to 53), there's no clean-up to do.


You should ship this only to 52, not 52-55. Unless there's no more time to uplift this pref disabling to beta 53? I don't think that's true.

And a reminder that 52 is ESR so this will need to go to the ESR branch too, or in a dot release there (just remember to mention this to ckprice during the release process).

Comment 22

[u408661]

Thanks. Moving the system add-on stuff to bug 1351082

Comment 23

[u408661]

Attachment: Beta disable patch

Comment 24

[u408661]

Attachment: Aurora disable patch

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

This will miss the beta 7 build (we already started it) but will likely land for beta 8 later this week.

Comment 26

[Julien Cristau [:jcristau]]

Tracking for esr52

Comment 27

[u408661]

Comment on attachment 8851792 [details] [diff] [review]
Beta disable patch

Approval Request Comment
[Feature/Bug causing the regression]: predictive prefetch
[User impact if declined]: causes false ad impressions
[Is this code covered by automated tests?]: yes (pref flip)
[Has the fix been verified in Nightly?]: yes (and has been flipped off many times before)
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple pref flip
[String changes made/needed]: none

Comment 28

[u408661]

Comment on attachment 8851793 [details] [diff] [review]
Aurora disable patch

Approval Request Comment
[Feature/Bug causing the regression]: predictive prefetch
[User impact if declined]: causes false ad impressions
[Is this code covered by automated tests?]: yes (pref flip)
[Has the fix been verified in Nightly?]: yes (and has been flipped off many times before)
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple pref flip
[String changes made/needed]: none

Comment 29

[hendrik schumacher]

Sorry if this is off-topic but we also investigated this and wondered why there are no http headers that signal prefetching (eg X-Moz: prefetch). It would be nice if someone could comment if such a header should be present and if we should file a separate bug for this.

Comment 30

[u408661]

Attachment: Bug 1349921 - don't prefetch is there's a query string

Review commit: https://reviewboard.mozilla.org/r/124824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124824/

Comment 31

[u408661]

The patch attached currently handles the query-string case, pretty straightforward. Based on the 3 bugs, this looks like this will get us at least 99% of the way there for the behavior we want (but I've been proven wrong before!)

I'm still investigating how to differentiate, eg, session restore vs. explicit user navigation for that portion.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Nick, see comment 29?

Comment 33

[Gabriel]

I don't have access to add comments on the code review, but:

(comment i tried to add to the line skipping cache for URLs with query string)
That was not the reason of the problem that we saw on https://bugzilla.mozilla.org/show_bug.cgi?id=1350005
There the request had a query string, was cacheable, but it was send over the network over and over, even though it was supposed to be in cache already.
I don't believe invalidating cache just because it has a query string is right...

Comment 34

[u408661]

(In reply to Gabriel from comment #33)
> I don't have access to add comments on the code review, but:
> 
> (comment i tried to add to the line skipping cache for URLs with query
> string)
> That was not the reason of the problem that we saw on
> https://bugzilla.mozilla.org/show_bug.cgi?id=1350005
> There the request had a query string, was cacheable, but it was send over
> the network over and over, even though it was supposed to be in cache
> already.
> I don't believe invalidating cache just because it has a query string is
> right...

This doesn't invalidate the cache. Just marks the resource as something we shouldn't prefetch. (The flag is perhaps poorly-named, but that's neither here nor there for the functionality we're looking for).

Comment 35

[u408661]

(In reply to hendrik schumacher from comment #29)
> Sorry if this is off-topic but we also investigated this and wondered why
> there are no http headers that signal prefetching (eg X-Moz: prefetch). It
> would be nice if someone could comment if such a header should be present
> and if we should file a separate bug for this.

Yes, it is mostly off-topic for this bug. Originally, the decision was explicitly made to *not* send any special header for prefetches in order for them to look to the server like any other request (so that it doesn't do "special", possibly-breaking things to prefetched resources). If you wish to re-open that discussion, please open a new bug.

Comment 36

[Patrick McManus [:mcmanus]]

Comment on attachment 8852636 [details]
Bug 1349921 - don't prefetch is there's a query string

https://reviewboard.mozilla.org/r/124824/#review127446

r+ for the diff, but lets move it to another bug and number that deals with doing this on m-c rather than the deployment problem so its clear in the vcs.. and then we can also open another bug that deals with the context of what initiates the prefetches as we discussed offline. thanks!

Comment 37

[hendrik schumacher]

(In reply to Nicholas Hurley [:nwgh][:hurley] (also [e-mail]) from comment #35)
> (In reply to hendrik schumacher from comment #29)
> > Sorry if this is off-topic but we also investigated this and wondered why
> > there are no http headers that signal prefetching (eg X-Moz: prefetch). It
> > would be nice if someone could comment if such a header should be present
> > and if we should file a separate bug for this.
> 
> Yes, it is mostly off-topic for this bug. Originally, the decision was
> explicitly made to *not* send any special header for prefetches in order for
> them to look to the server like any other request (so that it doesn't do
> "special", possibly-breaking things to prefetched resources). If you wish to
> re-open that discussion, please open a new bug.

Thanks for the clarification!

Comment 38

[Kohei Yoshino]

Posted a site compatibility note for the issue: https://www.fxsitecompat.com/en-CA/docs/2017/predictive-prefetch-makes-duplicate-http-requests-in-iframes-causing-false-ad-impressions/

Comment 39

[Gabriel]

Thanks so much for answering our concerns (and for the best browser!)

Feel free to add us (from bug 1350005) to the new bugs and I will try my best to help further validate the changes when they are ready.

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8851793 [details] [diff] [review]
Aurora disable patch

Landing the pref flip to disable prefetch on 54 aurora.

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8851792 [details] [diff] [review]
Beta disable patch

Let's make sure this lands for 53 beta 9.

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

Are you also intending to disable this for esr?

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0cba1d941e64

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a46fdd9244c6

Comment 45

[Dennis Riedel]

Dear Mozillians,
as we, as a marketer/publisher, are affected by this bug in our operations and are facing recourse/compensation claims for the month of March and campaign cancelations/postponements for April, can you confirm or give details on the rollout of the configuration change for this flag ("network.predictor.enable-prefetch")? Are the following the earliest shipping dates for the changed flag?

2017-04-17: Firefox 56; Firefox 55; Firefox 54
2017-04-18: Firefox 53
2017-04-18: Firefox 45.9; 52.1 (ESR)

Source: https://wiki.mozilla.org/RapidRelease/Calendar

Thank you in advance and best regards,
Dennis

Comment 46

[u408661]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #42)
> Are you also intending to disable this for esr?

That should be taken care of by the system add-on (bug 1351082). So yes, but not here :)

Comment 47

[u408661]

Comment on attachment 8851792 [details] [diff] [review]
Beta disable patch

Looks like we want to land the patch on esr52. This version should apply cleanly there.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: see "user impact if declined" - we're doing this on all branches numbered 52+. This is so we don't have to worry about installing the system addon out-of-band for ESR.
User impact if declined: incorrect ad impressions
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 48

[Julien Cristau [:jcristau]]

Comment on attachment 8851792 [details] [diff] [review]
Beta disable patch

prefetch disable for esr52

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/c6897adc4037

Comment 52

[u408661]

Based on the pref-off patches landed here, the system addon being deployed over in bug 1351082, and the larger fix patch landed in bug 1353082, I'm going to resolve this bug. If we see similar behavior in different situations, let's open a new bug so we don't start getting things confused. Thanks.