921023 - Cannot enable Mixed Content Blocking after disabling it

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[u421692]

Environment:
Device: LG Nexus 4 (Android 4.2.2)
Build: Nightly 26.0a1 (25-09-2013)

Steps to reproduce:
1. Open https://people.mozilla.com/~bsterne/tests/62178/test.html
2. Tap on the icon for mixed content notification and tap on Disable protection button
3. Tap again on the icon for mixed content notification and tap on Enable protection button

Expected result:
The protection is enabled and the content is blocked.

Actual result:
The protection is still disabled and the content is not blocked.

Regression window:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2ab07dec6404&tochange=fb2318875cd4

This Bug 902156 - Persist "disable protection" option for Mixed Content Blocker changed the behaviour of mixed content blocking.

Comment 1

[Aaron Train [:aaronmt]]

What is the intended behaviour here? Is this working as expected?

Comment 2

[u421692]

The behaviour should be as presented in bug 860581, pressing the button should Enable protection. "Disable protection" should remain the same if refreshing the page.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

From your explanation it's not completely clear if you refresh the page. If you refresh, the protection should be enabled again. Requesting more info from tanvi for verification.

Comment 4

[Tanvi Vyas[:tanvi]]

The Mixed Content Blocker UI for Android is a little bit different than the one for desktop.  For Desktop, we don't have a "enable protection"button yet.  Adding need-info to Margaret, who worked on the MCB Android front-end code.

Comment 5

[Aaron Train [:aaronmt]]

On Nightly (on Android), it seems like on refresh we keep things disabled and opting in to enable protection again does nothing – that seems broken. Not seeing any errors in console.

Comment 6

[Aaron Train [:aaronmt]]

Nominating for tracking as a regression for what seems to be a situation in that you can not re-enable protection after displaying mixed content even after a page reload.

Comment 7

[:Margaret Leibovic]

(In reply to Tanvi Vyas [:tanvi] from comment #4)
> The Mixed Content Blocker UI for Android is a little bit different than the
> one for desktop.  For Desktop, we don't have a "enable protection"button
> yet.  Adding need-info to Margaret, who worked on the MCB Android front-end
> code.

Yes, it looks like this has regressed with bug 902156. Previously, reloading the page would re-enable mixed content blocking, but that's no longer the case.

Tanvi, can you advise what we should do here? Is there a flag we can use to re-enable mixed content blocking on a page, similarly to how we disable it?

For some context, here's where we currently implement this:
http://hg.mozilla.org/mozilla-central/annotate/192461a9dfe8/mobile/android/chrome/content/browser.js#l1319

Comment 8

[Tanvi Vyas[:tanvi]]

(In reply to :Margaret Leibovic from comment #7)
> Yes, it looks like this has regressed with bug 902156. Previously, reloading
> the page would re-enable mixed content blocking, but that's no longer the
> case.
> 
> Tanvi, can you advise what we should do here? Is there a flag we can use to
> re-enable mixed content blocking on a page, similarly to how we disable it?
> 
Oh boy!  What version of Firefox Android did MCB go out in?  Is it the stable release or on beta right now?

The patch to bug 902156 was requested by many users/developers and mozillians.  If a user decided to disable protection on a site, we wanted it to persist for at least as long as they were on the site.  Once they left, we would go back to the original blocked state.  We did this by persisting the mMixedContentChannel if the domain has not changed instead of setting it to null - https://hg.mozilla.org/releases/mozilla-beta/rev/fd3457697fc9.  This was implemented in Firefox 24.

In order to re-enable protection, you want to set mMixedContentChannel back to null, since a refresh will no longer be sufficient.  You can find this in this idl - http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShell.idl#818

On another note, are there mochitests for Android's MCB?  Last I checked (probably more than 6 months ago), mochitests on Android don't support SSL, making it impossible to right tests for MCB.  Maybe that has changed.  A mochitest for the re-enable protection feature on Android would have caught this regression.

Comment 9

[:Margaret Leibovic]

(In reply to Tanvi Vyas [:tanvi] from comment #8)
> (In reply to :Margaret Leibovic from comment #7)
> > Yes, it looks like this has regressed with bug 902156. Previously, reloading
> > the page would re-enable mixed content blocking, but that's no longer the
> > case.
> > 
> > Tanvi, can you advise what we should do here? Is there a flag we can use to
> > re-enable mixed content blocking on a page, similarly to how we disable it?
> > 
> Oh boy!  What version of Firefox Android did MCB go out in?  Is it the
> stable release or on beta right now?

Fortunately it's still in beta, and we haven't had much feedback that people actually run into mixed content on mobile, so this isn't the end of the world :)

> The patch to bug 902156 was requested by many users/developers and
> mozillians.  If a user decided to disable protection on a site, we wanted it
> to persist for at least as long as they were on the site.  Once they left,
> we would go back to the original blocked state.  We did this by persisting
> the mMixedContentChannel if the domain has not changed instead of setting it
> to null - https://hg.mozilla.org/releases/mozilla-beta/rev/fd3457697fc9. 
> This was implemented in Firefox 24.
> 
> In order to re-enable protection, you want to set mMixedContentChannel back
> to null, since a refresh will no longer be sufficient.  You can find this in
> this idl -
> http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShell.
> idl#818

Thanks for the pointer.

> On another note, are there mochitests for Android's MCB?  Last I checked
> (probably more than 6 months ago), mochitests on Android don't support SSL,
> making it impossible to right tests for MCB.  Maybe that has changed.  A
> mochitest for the re-enable protection feature on Android would have caught
> this regression.

I haven't heard any update on this. I can ask gbrown from the A-team if he knows. A test for this definitely would be good :(

Comment 10

[:Margaret Leibovic]

Attachment: patch

Tanvi's suggestion worked. Thanks!

Comment 11

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/812f908502ce

Comment 12

[:Margaret Leibovic]

Comment on attachment 812349 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 902156 
User impact if declined: cannot re-enable mixed content blocking for a page from the button in the popup
Testing completed (on m-c, etc.): just landed on fx-team
Risk to taking this patch (and alternatives if risky): low-risk small change, only affects mixed content blocking notification popup
String or IDL/UUID changes made by this patch: none

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/812f908502ce

Comment 14

[Tanvi Vyas[:tanvi]]

Mihai, are you able to verify MCB features on Android as well as Desktop?  Or is there another QA resource that can help with Android.  Thanks!

Comment 15

[Mihai Morar, (:MihaiMorar)]

Android is not in my activity area so I'm moving this issue to Mihai Pop who has the ncessary resources to test this issue.

Comment 16

[Aaron Train [:aaronmt]]

This is fixed.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f8a5867aef49
https://hg.mozilla.org/releases/mozilla-beta/rev/db9a81f5ff91

Comment 18

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 25 Beta 6 and Firefox for Android 26 Aurora (2013-10-08)
Device: Samsung Galaxy Nexus
Android 4.1.1

Comment 19

[Geoff Brown [:gbrown]]

(In reply to :Margaret Leibovic from comment #9)
> (In reply to Tanvi Vyas [:tanvi] from comment #8)
> > On another note, are there mochitests for Android's MCB?  Last I checked
> > (probably more than 6 months ago), mochitests on Android don't support SSL,
> > making it impossible to right tests for MCB.  Maybe that has changed.  A
> > mochitest for the re-enable protection feature on Android would have caught
> > this regression.
> 
> I haven't heard any update on this. I can ask gbrown from the A-team if he
> knows. A test for this definitely would be good :(

I wasn't aware of this, but yes, apparently there are issues with ssl in Android mochitests. :jmaher thinks that the problem is that "ssltunnel isn't setup all the way". I could not find a related bug.