Closed Bug 1373780 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
DOM: Security
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- disabled
firefox57 --- fixed

People

(Reporter: aryx, Assigned: allstars.chh)

References

Details

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

Attachments

(5 files, 3 obsolete files)

temporarily disable test
711 bytes, patch
gbrown
: review+
Details | Diff | Splinter Review
Part 2: remove calling nsContentUtils::CanLoadImage().
1.64 KB, patch
jdm
: review+
Details | Diff | Splinter Review
Part 4: remove imageset.https.sub.html.ini
951 bytes, patch
annevk
: review+
Details | Diff | Splinter Review
Part 3: add isImgSet argument. v2
11.45 KB, patch
jdm
: review+
Details | Diff | Splinter Review
Part 1: Added smiley.png.headers v2
2.59 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review 
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
Priority: -- → P3
Whiteboard: [domsecurity-intermittent]
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]
Status: NEW → ASSIGNED
: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)
(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)
Attachment #8880356 - Flags: review?(gbrown) → review+
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]
Attached patch Part 3: add isImgSet argument (obsolete) — — Splinter Review 
updated commit message.
Attachment #8899699 - Attachment is obsolete: true
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)
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)
I don't understand Part 1. Can you explain how that relates to this bug?
Attachment #8899700 - Flags: review?(annevk) → review+
(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
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 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)
Attachment #8899698 - Flags: review?(bugs) → review?(josh)
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+
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 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+
(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
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+
updated MANIFEST.json
Attachment #8899697 - Attachment is obsolete: true
Attachment #8900606 - Flags: review+
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
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)
(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)
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.

Attachment

General

Creator:
Created:
Updated:
Size: