Closed Bug 1198438 Opened 6 years ago Closed 6 years ago

nsIContentPolicy should not modify srcset

Categories

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

43 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: jonalmeida, Assigned: smaug)

References

Details

Attachments

(4 files, 1 obsolete file)

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
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?
Flags: needinfo?(john)
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.
Flags: needinfo?(john)
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.
Flags: needinfo?(snorp)
I'm not sure what the right thing is here. Smaug?
Flags: needinfo?(snorp) → needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Thanks for the quick reply. Smaug, is there someone on your team who could help fix this test?
Flags: needinfo?(bugs)
I guess I could
Assignee: nobody → bugs
Flags: needinfo?(bugs)
I assume this is only about test_srcset_pref.html
hmm, that test contentpolicy is for Android only :/
ah, also test_picture_pref.html test
Attached patch wip (obsolete) — Splinter Review
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.
Attachment #8663911 - Flags: feedback?(jonalmeida942)
Thanks smaug! Trying this out right now.
Attached file 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.
Flags: needinfo?(bugs)
Comment on attachment 8663911 [details] [diff] [review]
wip

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

See comment 13.
Attachment #8663911 - Flags: feedback?(jonalmeida942)
logs don't really help, but I need some way to test this on desktop.
Flags: needinfo?(bugs)
Would really help here if the contentpolicy would work on desktop builds.
/me modifies the test contentpolicy
Attached patch test fixesSplinter Review
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).
Attachment #8663911 - Attachment is obsolete: true
Attachment #8664557 - Flags: feedback?
Is the last patch correct? It looks like the same one at that was marked obsolete?
Flags: needinfo?(bugs)
It is correct. I rewrote the fix after I had the test contentpolicy working, and the fix looked the same again.
Flags: needinfo?(bugs)
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.
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.
:(

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.
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 on attachment 8664557 [details] [diff] [review]
test fixes

Verified working on local builds! \o/

NI for smaug to land :D
Flags: needinfo?(bugs)
Attachment #8664557 - Flags: feedback? → feedback+
Comment on attachment 8664557 [details] [diff] [review]
test fixes

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

LGTM
Attachment #8664557 - Flags: review+
Flags: needinfo?(bugs)
https://hg.mozilla.org/mozilla-central/rev/2b5a7e0e2082
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.