Cannot enable Mixed Content Blocking after disabling it

VERIFIED FIXED in Firefox 25

Status

()

VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: u421692, Assigned: Margaret)

Tracking

({regression, reproducible})

27 Branch
Firefox 27
ARM
Android
regression, reproducible
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24 unaffected, firefox25+ verified, firefox26+ verified, firefox27 verified, fennec25+)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Keywords: regression
What is the intended behaviour here? Is this working as expected?
Flags: needinfo?(mozilla)
(Reporter)

Comment 2

5 years ago
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.
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.
Flags: needinfo?(mozilla) → needinfo?(tanvi)

Comment 4

5 years ago
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.
Flags: needinfo?(tanvi) → needinfo?(margaret.leibovic)
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.
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.
tracking-fennec: --- → ?

Updated

5 years ago
Keywords: reproducible
(Assignee)

Comment 7

5 years ago
(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
Flags: needinfo?(margaret.leibovic)
(Assignee)

Updated

5 years ago
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → 25+
status-firefox24: --- → unaffected
status-firefox25: --- → affected
(Assignee)

Updated

5 years ago
Blocks: 902156, 860581

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
(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 :(
Flags: needinfo?(gbrown)
(Assignee)

Comment 10

5 years ago
Created attachment 812349 [details] [diff] [review]
patch

Tanvi's suggestion worked. Thanks!
Attachment #812349 - Flags: review?(mark.finkle)
Attachment #812349 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 12

5 years ago
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
Attachment #812349 - Flags: approval-mozilla-beta?
Attachment #812349 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/812f908502ce
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27

Comment 14

5 years ago
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!
Flags: needinfo?(mihai.morar)
Keywords: verifyme
QA Contact: mihai.morar
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.
Flags: needinfo?(mihai.morar) → needinfo?(mihai.g.pop)
QA Contact: mihai.morar → mihai.g.pop
This is fixed.
Status: RESOLVED → VERIFIED
status-firefox27: affected → verified
Flags: needinfo?(mihai.g.pop)

Updated

5 years ago
Keywords: verifyme

Updated

5 years ago
tracking-firefox25: --- → +
tracking-firefox26: --- → +

Updated

5 years ago
Attachment #812349 - Flags: approval-mozilla-beta?
Attachment #812349 - Flags: approval-mozilla-beta+
Attachment #812349 - Flags: approval-mozilla-aurora?
Attachment #812349 - Flags: approval-mozilla-aurora+
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
status-firefox25: fixed → verified
status-firefox26: fixed → verified
(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.
Flags: needinfo?(gbrown)
You need to log in before you can comment on or make changes to this bug.