1373780 - Intermittent TEST-UNEXPECTED-PASS | /mixed-content/imageset.https.sub.html | Makes sure imageset blockable resources are not downloaded - expected FAIL

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P3)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=e886bf1ba8fceb67c705b3951bd2da2c72af1a59&filter-searchStr=dd7bb8ebe470216ac580dbf2997094d198fc0112&tochange=706d0255f5a55fa19e2658f6af03c477c53c7d34&selectedJob=107666450 has wpt 3 failures on Windows 7 VM opt. These should be from bug 1267075, but none of the retriggers for that push turned orange.

TEST-UNEXPECTED-PASS | /mixed-content/imageset.https.sub.html | Makes sure imageset blockable resources are not downloaded - expected FAIL

05:03:24     INFO - PID 1344 | 1497614604916	Marionette	INFO	Listening on port 2828
05:03:25     INFO - PID 1344 | 1497614605267	Marionette	DEBUG	loaded listener.js
05:03:25     INFO - PID 1344 | 1497614605302	Marionette	DEBUG	Received DOM event "beforeunload" for "about:blank"
05:03:25     INFO - PID 1344 | 1497614605958	Marionette	DEBUG	Received DOM event "pagehide" for "about:blank"
05:03:25     INFO - PID 1344 | 1497614605959	Marionette	DEBUG	Received DOM event "unload" for "about:blank"
05:03:25     INFO - PID 1344 | 1497614605968	Marionette	DEBUG	Received DOM event "DOMContentLoaded" for "http://web-platform.test:8000/testharness_runner.html"
05:03:25     INFO - PID 1344 | 1497614605972	Marionette	DEBUG	Received DOM event "pageshow" for "http://web-platform.test:8000/testharness_runner.html"
05:03:26     INFO - TEST-START | /mixed-content/imageset.https.sub.html
05:03:26     INFO - PID 1344 | 1497614606095	Marionette	DEBUG	Received DOM event "beforeunload" for "http://web-platform.test:8000/testharness_runner.html"
05:03:26     INFO - PID 1344 | 1497614606158	Marionette	DEBUG	Received DOM event "pagehide" for "http://web-platform.test:8000/testharness_runner.html"
05:03:26     INFO - PID 1344 | 1497614606158	Marionette	DEBUG	Received DOM event "unload" for "http://web-platform.test:8000/testharness_runner.html"
05:03:26     INFO - PID 1344 | 1497614606166	Marionette	DEBUG	Received DOM event "DOMContentLoaded" for "https://web-platform.test:8443/testharness_runner.html"
05:03:26     INFO - PID 1344 | 1497614606169	Marionette	DEBUG	Received DOM event "pageshow" for "https://web-platform.test:8443/testharness_runner.html"
05:03:26     INFO - PID 1344 | 1497614606257	Marionette	DEBUG	loaded listener.js
05:03:26     INFO - 
05:03:26     INFO - TEST-UNEXPECTED-PASS | /mixed-content/imageset.https.sub.html | Makes sure imageset blockable resources are not downloaded - expected FAIL
05:03:26     INFO - TEST-INFO | expected FAIL
05:03:26     INFO - TEST-OK | /mixed-content/imageset.https.sub.html | took 511ms

Comment 1

[Intermittent Failures Robot]

38 failures in 147 pushes (0.259 failures/push) were associated with this bug yesterday.   

Repository breakdown:
* autoland: 23
* mozilla-inbound: 14
* mozilla-central: 1

Platform breakdown:
* windows8-64: 14
* windows7-32-vm: 14
* osx-10-10: 10

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1373780&startday=2017-06-16&endday=2017-06-16&tree=all

Comment 2

[Intermittent Failures Robot]

52 failures in 814 pushes (0.064 failures/push) were associated with this bug in the last 7 days. 

This is the #37 most frequent failure this week.  

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. ** 

Repository breakdown:
* autoland: 33
* mozilla-inbound: 16
* mozilla-central: 3

Platform breakdown:
* windows8-64: 23
* windows7-32-vm: 17
* osx-10-10: 10
* linux64-ccov: 1
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1373780&startday=2017-06-12&endday=2017-06-18&tree=all

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

:allstars.chh, thanks for working on this bug, this is an opt only failure and on web-platform-tests e10s chunk 3.

Let me know if you need additional information as to how this is run, or to get something running in order to debug easier.

Comment 4

[Intermittent Failures Robot]

22 failures in 151 pushes (0.146 failures/push) were associated with this bug yesterday.   

Repository breakdown:
* autoland: 8
* try: 7
* mozilla-inbound: 6
* mozilla-central: 1

Platform breakdown:
* windows8-64: 9
* windows7-32-vm: 7
* linux64: 4
* osx-10-10: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1373780&startday=2017-06-19&endday=2017-06-19&tree=all

Comment 5

[Intermittent Failures Robot]

22 failures in 173 pushes (0.127 failures/push) were associated with this bug yesterday.   

Repository breakdown:
* autoland: 12
* mozilla-inbound: 6
* mozilla-central: 4

Platform breakdown:
* windows8-64: 10
* windows7-32-vm: 8
* osx-10-10: 2
* windows10-64-vm: 1
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1373780&startday=2017-06-20&endday=2017-06-20&tree=all

Comment 6

[Intermittent Failures Robot]

30 failures in 170 pushes (0.176 failures/push) were associated with this bug yesterday.   

Repository breakdown:
* autoland: 12
* mozilla-inbound: 11
* mozilla-central: 7

Platform breakdown:
* windows8-64: 14
* windows7-32-vm: 8
* osx-10-10: 4
* windows10-64-vm: 1
* linux64-ccov: 1
* linux64: 1
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1373780&startday=2017-06-21&endday=2017-06-21&tree=all

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

:allstars.chh, do you have any updates for this bug, the failure rate is increasing and I would like to see this resolved in the next day or two or we can temporarily disable this to give you more time.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Joel Maher ( :jmaher) from comment #7)
> :allstars.chh, do you have any updates for this bug, the failure rate is
> increasing and I would like to see this resolved in the next day or two or
> we can temporarily disable this to give you more time.

Probably that is working now as expected because of the imageLoader change.
Yoshi, what do you think?

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

(In reply to Joel Maher ( :jmaher) from comment #7)
> :allstars.chh, do you have any updates for this bug, the failure rate is
> increasing and I would like to see this resolved in the next day or two or
> we can temporarily disable this to give you more time.

Disable it sounds good to me, I am still working on it, and I'll be travelling in the following days.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> Probably that is working now as expected because of the imageLoader change.
> Yoshi, what do you think?
I am not sure either, the problem is it's intermittent, I am still checking my patches increase the failure (or success) rate.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: temporarily disable test

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bcb56297d04
/mixed-content/imageset.https.sub.html. temporarily disable. r=gbrown

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/0bcb56297d04

Comment 13

[Intermittent Failures Robot]

21 failures in 175 pushes (0.12 failures/push) were associated with this bug yesterday.   

Repository breakdown:
* autoland: 9
* mozilla-inbound: 7
* mozilla-central: 5

Platform breakdown:
* windows8-64: 12
* windows7-32-vm: 5
* osx-10-10: 3
* linux64-ccov: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1373780&startday=2017-06-22&endday=2017-06-22&tree=all

Comment 14

[Intermittent Failures Robot]

102 failures in 892 pushes (0.114 failures/push) were associated with this bug in the last 7 days. 

This is the #14 most frequent failure this week. 

** This failure happened more than 75 times this week! Resolving this bug is a very high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 1 week, the affected test(s) may be disabled. **  

Repository breakdown:
* autoland: 41
* mozilla-inbound: 33
* mozilla-central: 19
* try: 9

Platform breakdown:
* windows8-64: 46
* windows7-32-vm: 31
* osx-10-10: 12
* linux64: 6
* linux64-ccov: 3
* windows10-64-vm: 2
* linux32: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1373780&startday=2017-06-19&endday=2017-06-25&tree=all

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 1: Added smiley.png.headers

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 2: remove calling nsContentUtils::CanLoadImage().

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 3: add isImgSet argument

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 4: remove imageset.https.sub.html.ini

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 3: add isImgSet argument

updated commit message.

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Comment on attachment 8899711 [details] [diff] [review]
Part 3: add isImgSet argument

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

Hi smaug
I found image preloading is done in bug 1067345 however there's no reviewer for it :-|
Since it's related to DOM so I ask for your feedback? first
(If you think I need feedback? from Parser peer please also let me know)

Hi Kate
This is also related to MCB, so I also ask for your feedback.

Thanks

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Comment on attachment 8899698 [details] [diff] [review]
Part 2: remove calling nsContentUtils::CanLoadImage().

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

Hi smaug
Bz reviewed my bug 1267075, but he's on PTO now.
Could you review this for me?

Thanks

Comment 22

[Anne (:annevk)]

I don't understand Part 1. Can you explain how that relates to this bug?

Comment 23

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

(In reply to Anne (:annevk) from comment #22)
> I don't understand Part 1. Can you explain how that relates to this bug?

Did you check my commit message in the patch?
Originally without this, the result from verifyNumberOfDownloads is always 0

Comment 24

[Anne (:annevk)]

Apologies, I just looked at the diff and didn't realize the patch included a commit message. Would be nice if the tooling around patches exposed that more clearly somehow.

So the reason you're including this is because there's currently two reasons it should fail: mixed content & lack of that header. And you want there to be just one reason for this to fail: mixed content.

That seems reasonable, but I still don't understand how that would make things flaky. It seems it should still deterministically fail either way.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8899711 [details] [diff] [review]
Part 3: add isImgSet argument

jdm, could you take a look? You're more familiar with picture and srcset than I am.

Comment 26

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Comment on attachment 8899697 [details] [diff] [review]
Part 1: Added smiley.png.headers

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

(In reply to Anne (:annevk) from comment #24)
> That seems reasonable, but I still don't understand how that would make things flaky.
I'll check the intermittents in the first place then back to you.
Thanks

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Comment on attachment 8899697 [details] [diff] [review]
Part 1: Added smiley.png.headers

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

Anne, I'll explain the intermittent timeout here, as I think the intermittent shouldn't happen now.
The latest preload_helper.js was modifed in
https://hg.mozilla.org/mozilla-central/diff/ce6451b2ea64/testing/web-platform/tests/preload/resources/preload_helper.js

So let's rollback to the earlier version, for example, back to the version this intermittent start happening.
http://searchfox.org/mozilla-central/rev/824f4e083bc5a0961196261474acad5c10a38c1e/testing/web-platform/tests/preload/resources/preload_helper.js#15

At that time, the test used 'performance.getEntriesByName' to test, which doesn't need same-origin check
However, the latest version now,
http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/testing/web-platform/tests/preload/resources/preload_helper.js#17

uses 'entry.transferSize' to test
and this requires 'Same-Origin' check, as explained in the commit message.

So this Part 1 patch fixes the current failure in imageset.https.sub.html in both Firefox and Chrome.
It's not directly related to the intermittent when this bug is created.

Back to the intermittent, the unexpected-OK means the img_src is successfully fetched, whereas img_srcset and <picture> are blocked,
this is our current correct behavior of Mixed-content-blocker.

But this should also imply one thing: image preloading didn't happen.
I check the code, 
http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/parser/html/nsHtml5TreeOpExecutor.cpp#998

I think the preloading didn't happen because the function 'ShouldPreloadURI(uri)' returned false
and the reason it returned false is because 'we have loaded this URI before'.

And this is related to my fix in bug 1267075.
As in my bug 1267075, I removed the call to CanLoadImage,
https://hg.mozilla.org/mozilla-central/diff/8feacdbd43fa/dom/base/nsImageLoadingContent.cpp

So before bugf 1267075, images to be blocked would be bailed out in CanLoadImage function
https://hg.mozilla.org/mozilla-central/diff/8feacdbd43fa/dom/base/nsImageLoadingContent.cpp#l1.16

now it will go into imgLoader code, and asyncOpen2 will do the security check instead,
and more importantly, this will create more image caches!
So this increases the chances that smiley.png is cached.

And also smiley.png is used a lot in the web-platform-tests. 
http://searchfox.org/mozilla-central/search?q=smiley.png&path=

I think above explains the intermittent, and my Part 3 patch should fix the intermittent now.

Sorry for the long long explanation. :P
Thanks

Comment 28

[Anne (:annevk)]

Comment on attachment 8899697 [details] [diff] [review]
Part 1: Added smiley.png.headers

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

Thanks for looking into it. I still don't quite grasp all the nuances of the performance API (and haven't studied it in detail), but at least it's clear how the intermittent bit is not related.

Comment 29

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

(In reply to Anne (:annevk) from comment #28)
> Comment on attachment 8899697 [details] [diff] [review]
> Part 1: Added smiley.png.headers
> 
> Review of attachment 8899697 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for looking into it. I still don't quite grasp all the nuances of the
> performance API (and haven't studied it in detail), but at least it's clear
> how the intermittent bit is not related.

Hi Anne
The call to get TranserSize is in
http://searchfox.org/mozilla-central/source/dom/performance/PerformanceResourceTiming.h#143,
and the 'TimingAllowCheck()' is used to check if they are same origin.

There's one problem I'd like to mention, but no related to this bug.
I might not 100% sure how the working model of web-platform-tests for different browser vendor is,
but in this case,
I think somehow the commit 
https://hg.mozilla.org/mozilla-central/diff/ce6451b2ea64/testing/web-platform/tests/preload/resources/preload_helper.js

affects some other wpt tests, which also affects some other browser vendors,
and we didn't catch this problem because we mark 'expected: FAIL' in this test from the beginning,
and I don't know why other browser vendors also didn't notice this, because I think
it's Chrome team wrote this test,
https://chromium.googlesource.com/chromium/src/+/babb7e6d7db134dab30bfa1419fc0b45744a2645%5E%21/
and was PASS in the begining,

However due to we also changed the preload_helper.js, now their tests is also failing now, perhaps there should be some working flows to prevent something like this happens again.

Thanks

Comment 30

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 3: add isImgSet argument. v2

Hi Josh
This is almost the same patch you feedbacked before, I just updated comments in the header file and changed this to a r? to you.

Since this patch is more like an image preloading fix, not directly related to MCB, so I cancel the feedback? from Kate, (also seems Kate is not arround)

Thanks

Comment 31

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 1: Added smiley.png.headers v2

updated MANIFEST.json

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f782467df88
Part 1: Added smiley.png.headers. r=annevk
https://hg.mozilla.org/integration/mozilla-inbound/rev/5585ec0d030c
Part 2: remove calling nsContentUtils::CanLoadImage(). r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/6852bc7a7671
Part 3: add isImgSet argument. r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/01e9d2d25323
Part 4: remove imageset.https.sub.html.ini. r=annevk

Comment 33

[Anne (:annevk)]

Yoshi, I think the best course of action is filing a bug against Chrome at https://crbug.com/new and mention this bug and the failure you found. They won't automatically notice a new failure. Nobody has tooling around that unfortunately so it requires manual notification.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f782467df88
https://hg.mozilla.org/mozilla-central/rev/5585ec0d030c
https://hg.mozilla.org/mozilla-central/rev/6852bc7a7671
https://hg.mozilla.org/mozilla-central/rev/01e9d2d25323

Comment 35

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

(In reply to Anne (:annevk) from comment #33)
> Yoshi, I think the best course of action is filing a bug against Chrome at
> https://crbug.com/new and mention this bug and the failure you found. They
> won't automatically notice a new failure. Nobody has tooling around that
> unfortunately so it requires manual notification.

https://bugs.chromium.org/p/chromium/issues/detail?id=758815