Missing plugin UI is broken

VERIFIED FIXED in Firefox 36



4 years ago
4 years ago


(Reporter: billm, Assigned: yury)



Windows 8.1
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox35 unaffected, firefox36+ verified, firefox37+ verified, firefox38+ verified, firefox-esr31 unaffected)



(1 attachment)

In Windows 8, using Nightly (non-e10s), Aurora, and beta:

1. Uninstall Flash
2. Visit http://www.homestarrunner.com/

See a plain gray box with no next

See "A plugin is needed to display this content."

This works correctly in release.

Comment 1

4 years ago
This artificial testcase WFM http://benjamin.smedbergs.us/tests/ctptests/missing-solo.html

But I can reproduce this on homestarrunner.com

I tried to debug this at http://hg.mozilla.org/mozilla-central/annotate/58ce6051edf5/browser/modules/PluginContent.jsm#l394 using the builtin browser toolbox. Oddly, we seem to end up skipping over hunks of code for no reason when I step through:

Stepping, I get to
and then the debugger immediately skips to

In any case, that's the place to keep poking at this.

Comment 2

4 years ago
After talking with jimb a bit, I'm going to try this in a self build to see if the symptoms still show up; but we think this might actually be some kind of JS execution bug. We almost ruled out debugger sourcemap errors because setting interior breakpoints don't hit anything.
I'm tracking 36+ in order to keep this on the radar while we figure out what's going on.

Benjamin - Looks like you're working on this. Can we assign this one to you?
status-firefox-esr31: --- → unaffected
tracking-firefox36: ? → +
tracking-firefox37: ? → +
tracking-firefox38: ? → +
Flags: needinfo?(benjamin)

Comment 4

4 years ago
I'm not going to commit to this, no. In the grand scheme of things, I'd probably ship with this regression except for the really weird bit about this looking like a JS execution/codegen bug.
Flags: needinfo?(benjamin)

Comment 5

4 years ago
A debug local build doesn't reproduce this issue.

Comment 6

4 years ago
A release local build doesn't reproduce either.
Maybe a regression window will help.
Keywords: regressionwindow-wanted

Comment 8

4 years ago
Nightly-debug linux doesn't reproduce either.

Comment 9

4 years ago
2014-10-01-03-02-05 nightly doesn't reproduce, so that's at least an early-bound on the bisection.
Here are the JS changes and other possible suspects from the regression range:

    8c7d2cd4fa06    Tom Schuster — Bug 1081660 - Remove property iterator from JSAPI. r=Waldo
    9702f1f2f133    Jason Orendorff — Bug 1083204 - Handlify js::GetPropertyKeys and Snapshot. r=efaust.
    a2458d9afc81    Jason Orendorff — Bug 1081280 - Rename BaseProxyHandler::keys -> getOwnEnumerablePropertyKeys. r=efaust.
    075cf4911854    Jason Orendorff — Bug 1081255 - Rewrite comments in jsproxy.h; reclassify the methods a bit. No change in behavior. r=efaust, r=bz, r=jwalden.
    9a7fd8fd00b3    Jason Orendorff — Bug 1082672, part 4 - Change XrayWrapper code to be able to resolve symbol-keyed methods. r=bz, r=bholley.
    f2214b9e3333    Jason Orendorff — Bug 1082672, part 3 - Add some more symbol support for DOM bindings. r=bz.
    bff9837442af    Jason Orendorff — Bug 1082672, part 2 - Change mozilla::dom::GetArrayIndexFromId to cope with symbols. r=bz.
    ac0d55e594f1    Jason Orendorff — Bug 1082672, part 1 - Add JSAPI macros JS_SYM_FN etc. to support defining functions with well-known symbol keys. r=Waldo.
    c0d3ea180fe0    Jason Orendorff — Add some test cases involving Symbol.iterator as a prelude to bug 918828. no_r=me, testonly.
    a758ca998666    Mike Hommey — Bug 609976 - Fold mozjs.dll back into xul.dll. r=ehsan
    b56d94c7261a    Jan de Mooij — Bug 987560 - Greatly refactor generator implementation. Patch mostly written by Andy Wingo. r=wingo
Flags: needinfo?(jdemooij)
 3:44.86 LOG: MainThread Bisector INFO Last good revision: ae1dfa192faf
 3:44.86 LOG: MainThread Bisector INFO First bad revision: 92c87e95915e
 3:44.86 LOG: MainThread Bisector INFO Pushlog:

20:10.04 LOG: MainThread Bisector INFO Last good revision: 9a7fd8fd00b3
20:10.04 LOG: MainThread Bisector INFO First bad revision: dc2d310c063c
20:10.04 LOG: MainThread Bisector INFO Pushlog:

I can not reproduce the issue in my local non-release build....
Keywords: regressionwindow-wanted
Ah! It looks like local builds will return false for Services.telemetry.canSend:
That's probably why we can't reproduce this in self-made builds.

Thanks Matthias!

Comment 14

4 years ago
If it really is a JS execution problem, then I would be suspicious of bug 987560, the generator changes, in that list. That was a very large change, and it touches code generation.
Jandem asked whether disabling javascript.options.baselinejit or javascript.options.ion fixed the problem, and local testing indicates that it makes no difference.
(In reply to Bill McCloskey (:billm) from comment #13)
> Ah! It looks like local builds will return false for
> Services.telemetry.canSend:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/
> Telemetry.cpp#2910
> That's probably why we can't reproduce this in self-made builds.

Yes, it repros locally if I make that return true. The problem is in _recordFlashPluginTelemetry:

      let state = this._getPluginInfo(plugin).fallbackType;

state is null, so add(state) throws "Not a number". Commenting out these lines "fixes" it.
Blocks: 1075088
Flags: needinfo?(jdemooij) → needinfo?(ydelendik)

Comment 17

4 years ago
It is safe to add something like:

if (state === null) state = Ci.nsIObjectLoadingContent.PLUGIN_UNSUPPORTED;

Eventually we will remove flash telemetry code.
Flags: needinfo?(ydelendik)


4 years ago
Assignee: nobody → ydelendik
Keywords: regression
Comment on attachment 8560695 [details] [diff] [review]
Guard against state==null

Review of attachment 8560695 [details] [diff] [review]:

By inspection, this looks good - though I'm not currently in a position to test this. We might want QA to bang the tires on this just to be sure.
Attachment #8560695 - Flags: review?(mconley) → review+


4 years ago
Keywords: checkin-needed

Comment 21

4 years ago
How about:
>+let state = this._getPluginInfo(plugin).fallbackType || Ci.nsIObjectLoadingContent.PLUGIN_UNSUPPORTED;
Last Resolved: 4 years ago
status-firefox38: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Yury, can you fill the uplift request for aurora & beta? Thanks
Flags: needinfo?(ydelendik)

Comment 24

4 years ago
Comment on attachment 8560695 [details] [diff] [review]
Guard against state==null

Approval Request Comment
[Feature/regressing bug #]: Bug 1075088
[User impact if declined]: Missing plugin UI 
[Describe test coverage new/current, TreeHerder]: in m-c, https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8831ddb0041, https://treeherder.mozilla.org/#/jobs?repo=try&revision=48881bbf0f06
[Risks and why]: Very low -- trivial patch
[String/UUID change made/needed]: none
Flags: needinfo?(ydelendik)
Attachment #8560695 - Flags: approval-mozilla-beta?
Attachment #8560695 - Flags: approval-mozilla-aurora?
Attachment #8560695 - Flags: approval-mozilla-beta?
Attachment #8560695 - Flags: approval-mozilla-beta+
Attachment #8560695 - Flags: approval-mozilla-aurora?
Attachment #8560695 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
Verified as fixed using the following environment:

FF 36.0b9
Build Id:20150212154903
OS: Win 8.1 x86
status-firefox36: fixed → verified
Verified as fixed on Win 8.1 64-bit using Firefox 37 beta 1 and Nightly 38.0a1.
status-firefox37: fixed → verified
status-firefox38: fixed → verified
You need to log in before you can comment on or make changes to this bug.