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

RESOLVED FIXED in Firefox 57

Status

()

Core
DOM: Security
P3
normal
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: aryx, Unassigned)

Tracking

({intermittent-failure})

Trunk
mozilla57
intermittent-failure
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 disabled, firefox57 fixed)

Details

(Whiteboard: [domsecurity-intermittent][stockwell fixed:other])

Attachments

(5 attachments, 3 obsolete attachments)

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

10 months ago
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
Priority: -- → P3
Whiteboard: [domsecurity-intermittent]

Comment 2

10 months ago
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
(Assignee)

Updated

10 months ago
Assignee: nobody → allstars.chh
: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.
Whiteboard: [domsecurity-intermittent] → [domsecurity-intermittent][stockwell needswork]

Comment 4

10 months ago
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

10 months ago
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
(Assignee)

Updated

10 months ago
Status: NEW → ASSIGNED

Comment 6

10 months ago
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
: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.
(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?
Flags: needinfo?(allstars.chh)
(Assignee)

Comment 9

10 months ago
(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.
Flags: needinfo?(allstars.chh)
Created attachment 8880356 [details] [diff] [review]
temporarily disable test
Attachment #8880356 - Flags: review?(gbrown)

Updated

10 months ago
Attachment #8880356 - Flags: review?(gbrown) → review+

Comment 11

10 months ago
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bcb56297d04
/mixed-content/imageset.https.sub.html. temporarily disable. r=gbrown
Whiteboard: [domsecurity-intermittent][stockwell needswork] → [domsecurity-intermittent][stockwell disabled]

Comment 13

10 months ago
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

10 months ago
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
(Assignee)

Comment 15

8 months ago
Created attachment 8899697 [details] [diff] [review]
Part 1: Added smiley.png.headers
(Assignee)

Comment 16

8 months ago
Created attachment 8899698 [details] [diff] [review]
Part 2: remove calling nsContentUtils::CanLoadImage().
(Assignee)

Comment 17

8 months ago
Created attachment 8899699 [details] [diff] [review]
Part 3: add isImgSet argument
(Assignee)

Comment 18

8 months ago
Created attachment 8899700 [details] [diff] [review]
Part 4: remove imageset.https.sub.html.ini
(Assignee)

Comment 19

8 months ago
Created attachment 8899711 [details] [diff] [review]
Part 3: add isImgSet argument

updated commit message.
Attachment #8899699 - Attachment is obsolete: true
(Assignee)

Comment 20

8 months ago
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
Attachment #8899711 - Flags: feedback?(kmckinley)
Attachment #8899711 - Flags: feedback?(bugs)
(Assignee)

Updated

8 months ago
Attachment #8899697 - Flags: review?(annevk)
(Assignee)

Comment 21

8 months ago
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
Attachment #8899698 - Flags: review?(bugs)
(Assignee)

Updated

8 months ago
Attachment #8899700 - Flags: review?(annevk)

Comment 22

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

Updated

8 months ago
Attachment #8899700 - Flags: review?(annevk) → review+
(Assignee)

Comment 23

8 months ago
(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

8 months ago
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

8 months ago
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.
Attachment #8899711 - Flags: feedback?(bugs) → feedback?(josh)

Updated

8 months ago
Attachment #8899698 - Flags: review?(bugs) → review?(josh)
(Assignee)

Comment 26

8 months ago
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
Attachment #8899697 - Flags: review?(annevk)
Attachment #8899698 - Flags: review?(josh) → review+
Attachment #8899711 - Flags: feedback?(josh) → feedback+
(Assignee)

Comment 27

8 months ago
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
Attachment #8899697 - Flags: review?(annevk)

Comment 28

8 months ago
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.
Attachment #8899697 - Flags: review?(annevk) → review+
(Assignee)

Comment 29

8 months ago
(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
(Assignee)

Comment 30

8 months ago
Created attachment 8900564 [details] [diff] [review]
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
Attachment #8899711 - Attachment is obsolete: true
Attachment #8899711 - Flags: feedback?(kmckinley)
Attachment #8900564 - Flags: review?(josh)
Attachment #8900564 - Flags: review?(josh) → review+
(Assignee)

Comment 31

8 months ago
Created attachment 8900606 [details] [diff] [review]
Part 1: Added smiley.png.headers v2

updated MANIFEST.json
Attachment #8899697 - Attachment is obsolete: true
Attachment #8900606 - Flags: review+
(Assignee)

Updated

8 months ago
Keywords: leave-open

Comment 32

8 months ago
Pushed by yhuang@mozilla.com:
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

8 months ago
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.
Flags: needinfo?(allstars.chh)

Comment 34

8 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
status-firefox55: --- → unaffected
status-firefox56: --- → disabled
status-firefox-esr52: --- → unaffected
(Assignee)

Comment 35

8 months ago
(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
Flags: needinfo?(allstars.chh)

Updated

8 months ago
Whiteboard: [domsecurity-intermittent][stockwell disabled] → [domsecurity-intermittent][stockwell fixed:other]
You need to log in before you can comment on or make changes to this bug.