1198438 - nsIContentPolicy should not modify srcset

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonathan Almeida [:jonalmeida]]

Attachment: test_contentpolicy.patch

When using the content policy interface in Fennec, we can return Ci.nsIContentPolicy.ACCEPT to simulate no change. However this still fails some mochitests[1][2] which expect srcset to be undefined.

I've attached a sample patch that causes the issue. (See: test_contentpolicy.patch)

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f80ce1845481
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=848f778a9c50

Comment 1

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

Adding John since he worked on <picture> and srcset stuff.

John - We are attempting to add a nsIContentPolicy-based feature in bug 1170725. That patch causes some tests to fail. Jonathan tracked the failure down to just needing a simple, almost empty, nsIContentPolicy impl. Something about just having a content policy is causing the tests to fail. Is this expected?

Comment 2

[John Schoenick [:johns]]

This test is asserting that disabling dom.images.*.enabled prefs should result in the element losing those attributes, yet with this CSP somehow `image.srcset` is returning something. This could be do to e.g. a race condition where CSP touching the element is causing it to be initialized as if srcset is enabled prior to the pref being flipped. So they look like legit failures, but it will take some debugging to figure out why the pref isn't taking effect.

Comment 3

[:Margaret Leibovic]

snorp, could you help us look into this? Or should we ask someone from the DOM team to help out? It looks like this is the only thing blocking us from landing bug 1170725.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

I'm not sure what the right thing is here. Smaug?

Comment 5

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

So isn't comment 2 explaining what probably happens here. The ContentPolicy (not CSP!) probably causes one to access some img element, which means the interface object for it is created in the global scope.
And since getters/setters live in the interface object, changing the preference afterwards doesn't
hide them, even if a new img object was created.


Note, function runTest(iframe) { accesses prototypes from the main test page, not from iframe.

So, this looks very much like a bug in the test.

Comment 6

[:Margaret Leibovic]

Thanks for the quick reply. Smaug, is there someone on your team who could help fix this test?

Comment 7

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

I guess I could

Comment 8

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

I assume this is only about test_srcset_pref.html

Comment 9

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

hmm, that test contentpolicy is for Android only :/

Comment 10

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

ah, also test_picture_pref.html test

Comment 11

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

Attachment: wip

This is tested without the contentpolicy, but manually touching img objects before changing pref.
So, please test if this helps with the issue you're seeing.

Comment 12

[Jonathan Almeida [:jonalmeida]]

Thanks smaug! Trying this out right now.

Comment 13

[Jonathan Almeida [:jonalmeida]]

Attachment: log_from_wip_patch.log

Smaug, I'm still seeing the same tests fail with the wip patch. I've attached the logs from my run.

Comment 14

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8663911 [details] [diff] [review]
wip

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

See comment 13.

Comment 15

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

logs don't really help, but I need some way to test this on desktop.

Comment 16

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

Would really help here if the contentpolicy would work on desktop builds.

Comment 17

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

/me modifies the test contentpolicy

Comment 18

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

Attachment: test contentpolicy for desktop

Comment 19

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

Attachment: test fixes

So this should actually have the same fixes as the previous wip patch, and seems to work just fine locally when testing with the content policy for desktop.

Jonathan, can you still reproduce with Android. Since if this doesn't help, I don't really have any way to hack this (I don't have Android dev environment).

Comment 20

[Jonathan Almeida [:jonalmeida]]

Is the last patch correct? It looks like the same one at that was marked obsolete?

Comment 21

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

It is correct. I rewrote the fix after I had the test contentpolicy working, and the fix looked the same again.

Comment 22

[Jonathan Almeida [:jonalmeida]]

I've tested this with the test contentpolicy patch (for mobile) and my click-to-play images patch, and both of them still fail with the same errors.

Comment 23

[Jonathan Almeida [:jonalmeida]]

Just to be clear, it's the test_srcset_pref.html test which is still failing, but the test_picture_pref.html one passes now.

Other things we've tried include putting the `img` in an iframe (like the picture test) and running it like that.

Comment 24

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

:(

Could you check if the relevant prefs are already on on all the platforms (desktop,mobile,ffos,tb)?
If they are, even in release branches, perhaps we should just remove the tests.

Comment 25

[Jonathan Almeida [:jonalmeida]]

I tried pushing the patches to try to see if my local build env wasn't working correctly and it seems good! \o/

(All my Android 4.0 builds seem to fail regardless of what I'm pushing, but I think that's a separate issue when I reset my credentials)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=677392c33edb

I'm going to rebuild my tree and confirm if the tests pass there later in the day.

Comment 26

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8664557 [details] [diff] [review]
test fixes

Verified working on local builds! \o/

NI for smaug to land :D

Comment 27

[John Schoenick [:johns]]

Comment on attachment 8664557 [details] [diff] [review]
test fixes

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

LGTM

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b5a7e0e2082

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b5a7e0e2082