1186925 - Rewrite existing tests using the old TP/MCB shield

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Tim Taubert [:ttaubert] (inactive)]

We will remove the old shield and need to update existing tests.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: badcontent-tests-WIP.patch

work in progress, but I have a green looking try push with this and the current patch from 1175702 applied together: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb373cb3996a

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1186925 - Convert tests using bad-content notification to use gIdentityHandler;r=tanvi

Bug 1186925 - Convert tests using bad-content notification to use gIdentityHandler;r=tanvi

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Created attachment 8644660 [details]
> MozReview Request: Bug 1186925 - Convert tests using bad-content
> notification to use gIdentityHandler;r=tanvi
> 
> Bug 1186925 - Convert tests using bad-content notification to use
> gIdentityHandler;r=tanvi

Hi Tanvi, this patch is switching out the existing uses of bad-content in tests with a new function I added in head.js - assertMixedContentBlockingState.  This new function is meant to test the state of both the icon in the URL bar and also the attributes set in the Control Center.  Can you please have a look?

There were a few places where I had to directly switch references from the popup notification to gIdentityHandler just because it was either waiting on a condition or it was a todo() assertion.

There's also a tag for all of these tests now so we can run `./mach mochitest -f browser --tag mcb` to cover the feature.

Comment 4

[Tanvi Vyas[:tanvi]]

https://reviewboard.mozilla.org/r/15283/#review13703

::: browser/base/content/test/general/browser_bug822367.js:76
(Diff revision 1)
> -  var notification = PopupNotifications.getNotification("bad-content", gTestBrowser);
> +  assertMixedContentBlockingState(gTestBrowser, {activeLoaded: false, activeBlocked: false, passiveLoaded: false});

It sounds like passive loaded should be true here?  Since this is a mixed display/passive test?

::: browser/base/content/test/general/browser_bug822367.js:91
(Diff revision 1)
> -  ok(notification, "Mixed Content Doorhanger did appear for test 3");
> +  assertMixedContentBlockingState(gTestBrowser, {activeLoaded: false, activeBlocked: true, passiveLoaded: false});

This test is checking that passive is blcoked and then loads after a disable event.  Should we add a passiveBlocked state?

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Looks like this is missing fixes for:

browser/devtools/webconsole/test/browser_webconsole_block_mixedcontent_securityerrors.js
dom/base/test/browser_bug902350.js

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Tim Taubert [:ttaubert] from comment #5)
> Looks like this is missing fixes for:
> 
> browser/devtools/webconsole/test/
> browser_webconsole_block_mixedcontent_securityerrors.js
> dom/base/test/browser_bug902350.js

Got them now, thanks

Comment 7

[Tanvi Vyas[:tanvi]]

Comment on attachment 8644660 [details]
MozReview Request: Bug 1186925 - Convert tests using bad-content notification to use gIdentityHandler;r=tanvi

https://reviewboard.mozilla.org/r/15285/#review13743

::: browser/base/content/test/general/head.js:789
(Diff revision 1)
> +  is(classList.contains("mixedActiveBlocked"), activeBlocked,

Do you have to add an && !passiveLoaded here.  Wihtout it, you might end up failing this test when active is Blocked and passive is Loaded, sine the classList would say "mixedDisplayContentLoadedActiveBlocked" and not "mixedActiveBlocked".  Or will the classList contain both mixedDisplayContentLoadedActiveBlocked and mixedActiveBlocked in this case?

::: browser/base/content/test/general/head.js:771
(Diff revision 1)
> + *           passiveLoaded: true|false,

consider adding passiveBlocked: true|false

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Tanvi Vyas [:tanvi] from comment #4)
> https://reviewboard.mozilla.org/r/15283/#review13703
> 
> ::: browser/base/content/test/general/browser_bug822367.js:76
> (Diff revision 1)
> > -  var notification = PopupNotifications.getNotification("bad-content", gTestBrowser);
> > +  assertMixedContentBlockingState(gTestBrowser, {activeLoaded: false, activeBlocked: false, passiveLoaded: false});
> 
> It sounds like passive loaded should be true here?  Since this is a mixed
> display/passive test?

Hm for some reason when I pause the test there the lock is green with no icon.  Maybe this uncovered a bug?  Even if I just open a new tab during the test and navigate to https://test1.example.com/browser/browser/base/content/test/general/file_bug822367_2.html I see a green lock icon and the identity-box class is "verifiedDomain".

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Tanvi Vyas [:tanvi] from comment #7)
> Comment on attachment 8644660 [details]
> MozReview Request: Bug 1186925 - Convert tests using bad-content
> notification to use gIdentityHandler;r=tanvi
> 
> https://reviewboard.mozilla.org/r/15285/#review13743
> 
> ::: browser/base/content/test/general/head.js:789
> (Diff revision 1)
> > +  is(classList.contains("mixedActiveBlocked"), activeBlocked,
> 
> Do you have to add an && !passiveLoaded here.  Wihtout it, you might end up
> failing this test when active is Blocked and passive is Loaded, sine the
> classList would say "mixedDisplayContentLoadedActiveBlocked" and not
> "mixedActiveBlocked".  Or will the classList contain both
> mixedDisplayContentLoadedActiveBlocked and mixedActiveBlocked in this case?

You are right, the class list will only include one of mixedDisplayContentLoadedActiveBlocked or mixedActiveBlocked.  It's interesting that there aren't any failures even without that condition - it looks like there isn't an instance of `activeBlocked: true, passiveLoaded: true` in the tests after these changes (which would trigger the error).

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Tanvi Vyas [:tanvi] from comment #7)
> ::: browser/base/content/test/general/head.js:771
> (Diff revision 1)
> > + *           passiveLoaded: true|false,
> 
> consider adding passiveBlocked: true|false

OK, I'll add that state

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Hm for some reason when I pause the test there the lock is green with no
> icon.  Maybe this uncovered a bug?  Even if I just open a new tab during the
> test and navigate to
> https://test1.example.com/browser/browser/base/content/test/general/
> file_bug822367_2.html I see a green lock icon and the identity-box class is
> "verifiedDomain".

Ok figured out why the test is failing.  In https://people.mozilla.org/~tvyas/mixeddisplay.html, `state & nsIWebProgressListener.STATE_IS_BROKEN` is true, causing this condition [0] to run and showing the exclamation icon.

But in https://test1.example.com/browser/browser/base/content/test/general/file_bug822367_2.html, `state & nsIWebProgressListener.STATE_IS_SECURE` is true which causes this condition [1] to run.  And since there is no active blocked content it ends up showing the IDENTITY_MODE_DOMAIN_VERIFIED class (the green lock).

Is there any case that a page with passive loaded content should ever hit `state & nsIWebProgressListener.STATE_IS_SECURE` from the web progress listener?

[0]: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6886
[1]: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6884

Comment 12

[Tanvi Vyas[:tanvi]]

https://reviewboard.mozilla.org/r/15283/#review13751

::: browser/base/content/test/general/browser_bug822367.js:76
(Diff revision 1)
> -  var notification = PopupNotifications.getNotification("bad-content", gTestBrowser);
> +  assertMixedContentBlockingState(gTestBrowser, {activeLoaded: false, activeBlocked: false, passiveLoaded: false});

Passive is actually blocked here since the pref for mixed passive loading is set to true for this test.  Hence, all the current states should remain false, and later (when we have passiveBlocked support) passiveBlocked should be true.

Comment 13

[Tanvi Vyas[:tanvi]]

https://reviewboard.mozilla.org/r/15283/#review13703

> This test is checking that passive is blcoked and then loads after a disable event.  Should we add a passiveBlocked state?

As discussed on irc, we can't add passiveBlocked yet.  We need support for it in the code first.

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8644660 [details]
MozReview Request: Bug 1186925 - Convert tests using bad-content notification to use gIdentityHandler;r=tanvi

Bug 1186925 - Convert tests using bad-content notification to use gIdentityHandler;r=tanvi

Comment 15

[Tanvi Vyas[:tanvi]]

https://reviewboard.mozilla.org/r/15283/#review13759

::: browser/base/content/test/general/browser_bug822367.js:105
(Diff revision 2)
>    MixedTest4();

Now that we have the assert method, you could add an assertMixedContentBlockingSTate(gTestBrowser, {activeLoaded: true, activeBlocked: false, passiveLoaded: true}); just before this line.

::: browser/base/content/test/general/browser_bug822367.js:174
(Diff revision 2)
> +  waitForCondition(() => gIdentityHandler._identityBox.classList.contains("mixedActiveBlocked"), MixedTest6B, "waited too long for doorhanger");

Change the comment to something like "waited too long for control center to get mixed active blocked state" since we no longer have a doorhanger.

::: browser/base/content/test/general/browser_bug822367.js:198
(Diff revision 2)
>    MixedTestsCompleted();

Same as for Test3.  You can add an assert call here for mixed active loaded set to true.

Comment 16

[Tanvi Vyas[:tanvi]]

https://reviewboard.mozilla.org/r/15283/#review13765

::: browser/base/content/test/general/browser_bug902156.js:7
(Diff revision 2)
>   *    - Doorhanger to disable protection appears - we disable it

You may want to update all the uses of the word "doorhanger" in these tests since there is no longer a doorhanger.  If you want to do this as a followup bug, that is fine.

Comment 17

[Tanvi Vyas[:tanvi]]

https://reviewboard.mozilla.org/r/15283/#review13771

::: browser/devtools/webconsole/test/browser.ini:199
(Diff revision 2)
>  [browser_webconsole_allow_mixedcontent_securityerrors.js]

Can you add a mcb tag here too?

::: browser/devtools/webconsole/test/browser_webconsole_block_mixedcontent_securityerrors.js:41
(Diff revision 2)
>          text: "Blocked loading mixed active content \"http://example.com/\"",

This looks like a typo.  It should say loading mixed passive content.  But that is not realted to this bug, so I will file a followup for it.

::: browser/devtools/webconsole/test/browser_webconsole_block_mixedcontent_securityerrors.js:70
(Diff revision 2)
> -  });
> +    "Mixed Content state appeared on identity box");

Change to "Mixed Active Content state appeared on identity box".

Comment 18

[Tanvi Vyas[:tanvi]]

Comment on attachment 8644660 [details]
MozReview Request: Bug 1186925 - Convert tests using bad-content notification to use gIdentityHandler;r=tanvi

r+ with the changes mentioned in reviewboard.

Comment 19

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/15283/#review13771

> This looks like a typo.  It should say loading mixed passive content.  But that is not realted to this bug, so I will file a followup for it.

Yeah, looks like the webconsole message itself is printing 'active' so we will need to make sure that message gets updated with the test

Comment 20

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/15283/#review13765

> You may want to update all the uses of the word "doorhanger" in these tests since there is no longer a doorhanger.  If you want to do this as a followup bug, that is fine.

Replaced "Doorhanger to disable protection" with "Control Center button to disable protection"

Comment 21

[Brian Grinstead [:bgrins]]

Comment on attachment 8644660 [details]
MozReview Request: Bug 1186925 - Convert tests using bad-content notification to use gIdentityHandler;r=tanvi

Bug 1186925 - Convert tests using bad-content notification to use gIdentityHandler;r=tanvi

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 8644660 [details]
MozReview Request: Bug 1186925 - Convert tests using bad-content notification to use gIdentityHandler;r=tanvi

Not sure why reviewboard resubmitted for review.  Here's the interdiff: https://reviewboard.mozilla.org/r/15285/diff/2-3/

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2e33ac0bb341

Comment 24

[Phil Ringnalda (:philor)]

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