Closed Bug 1252870 Opened 5 years ago Closed 5 years ago

Unveiled failing assertions uncovered bug in plugin blocking

Categories

(Firefox :: General, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
e10s + ---
firefox47 --- fixed

People

(Reporter: mikedeboer, Assigned: jimm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
Duplicate of this bug: 1252681
Are you going to take this? I filed bug 1252870 and was planning on doing the touch up there.
(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
(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...
Assignee: nobody → jmathies
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.
Attachment #8725733 - Flags: review?(mdeboer)
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/
Attachment #8725733 - Flags: review?(mdeboer)
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?
Attachment #8725733 - Flags: review?(mdeboer)
(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.
(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 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/
Attachment #8725733 - Attachment description: MozReview Request: Bug 1252870 - Fixup a couple buggy plugin related browser chrome tests. r?mikedeboer → MozReview Request: Bug 1252870 - Fixup a couple buggy plugin related browser chrome tests. r=mikedeboer
Attachment #8725733 - Flags: review?(mdeboer)
Keywords: checkin-needed
Attachment #8725733 - Flags: review?(mdeboer) → review+
https://hg.mozilla.org/mozilla-central/rev/dc9c31033281
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.