1252870 - Unveiled failing assertions uncovered bug in plugin blocking

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mike de Boer [:mikedeboer]]

As explained in https://bugzilla.mozilla.org/show_bug.cgi?id=1241930#c11:

https://hg.mozilla.org/mozilla-central/diff/103700d6b355/browser/base/content/test/plugins/browser_blocking.js#l1.354

Which Jim introduced in bug 1129040: the return value of ContentTask.spawn is _always_ true-ish, because it returns a Promise; a `yield` was omitted.
`result` is expected to be `true`, but it's not.

See https://treeherder.mozilla.org/#/jobs?repo=try&revision=266edacc9b37

Comment 2

[Jim Mathies [:jimm]]

Are you going to take this? I filed bug 1252870 and was planning on doing the touch up there.

Comment 3

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #2)
> Are you going to take this? I filed bug 1252870 and was planning on doing
> the touch up there.

s/bug 1252870/bug 1252681

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to Jim Mathies [:jimm] from comment #3)
> (In reply to Jim Mathies [:jimm] from comment #2)
> > Are you going to take this? I filed bug 1252870 and was planning on doing
> > the touch up there.
> 
> s/bug 1252870/bug 1252681

Ah sorry, I didn't see you filed it! I don't think it'd be as efficient if I took this...

Comment 5

[Jim Mathies [:jimm]]

Attachment: MozReview Request: Bug 1252870 - Fixup a couple buggy plugin related browser chrome tests. r=mikedeboer

Review commit: https://reviewboard.mozilla.org/r/37611/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37611/

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 8725733 [details]
MozReview Request: Bug 1252870 - Fixup a couple buggy plugin related browser chrome tests. r=mikedeboer

hmm, there a mismatch in what one of these checks looks for and a comment.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 8725733 [details]
MozReview Request: Bug 1252870 - Fixup a couple buggy plugin related browser chrome tests. r=mikedeboer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37611/diff/1-2/

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8725733 [details]
MozReview Request: Bug 1252870 - Fixup a couple buggy plugin related browser chrome tests. r=mikedeboer

https://reviewboard.mozilla.org/r/37611/#review34177

Thanks, that was quick! :) Almost there...

::: browser/base/content/test/plugins/browser_blocking.js:350
(Diff revision 2)
> -  ok(result, "Plugin should be activated.");
> +  ok(!result, "Plugin should be activated.");

The big question here is that I'd expect this be `true` analogous to `objectLoadingContent.activated`. Can you explain this discrepency? If this really is correct, can you add a comment here with the explanation?

Comment 9

[Jim Mathies [:jimm]]

(In reply to Mike de Boer [:mikedeboer] from comment #8)
> Comment on attachment 8725733 [details]
> MozReview Request: Bug 1252870 - Fixup a couple buggy plugin related browser
> chrome tests. r?mikedeboer
> 
> https://reviewboard.mozilla.org/r/37611/#review34177
> 
> Thanks, that was quick! :) Almost there...
> 
> ::: browser/base/content/test/plugins/browser_blocking.js:350
> (Diff revision 2)
> > -  ok(result, "Plugin should be activated.");
> > +  ok(!result, "Plugin should be activated.");
> 
> The big question here is that I'd expect this be `true` analogous to
> `objectLoadingContent.activated`. Can you explain this discrepency? If this
> really is correct, can you add a comment here with the explanation?

It should be false, the plugin was blocklisted in this particular test case. I can update that text message to reflect that.

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Jim Mathies [:jimm] from comment #9)
> It should be false, the plugin was blocklisted in this particular test case.
> I can update that text message to reflect that.

Pfew! That'd be nice. r=me with that change!

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 8725733 [details]
MozReview Request: Bug 1252870 - Fixup a couple buggy plugin related browser chrome tests. r=mikedeboer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37611/diff/2-3/

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc9c31033281

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc9c31033281