Closed Bug 833936 Opened 9 years ago Closed 9 years ago

NPNVcontentsScaleFactor inadvertently not supported for NPN_GetValue() when called from OOP plugins

Categories

(Core :: Plug-ins, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox20 + fixed
firefox21 --- fixed

People

(Reporter: smichaud, Assigned: smichaud)

References

Details

Attachments

(2 files)

My patch for bug 785667 made plugins run properly in HiDPI mode.

In principle this included support for plugins to call NPN_GetValue(NPNVcontentsScaleFactor) in the browser.  But unfortunately I inadvertently put this support inside a #ifndef NP_NO_QUICKDRAW block for plugins that run out-of-process (by default all of them) -- which means that as of FF 18 (when support was dropped for the NPAPI QuickDraw drawing model) this support doesn't get compiled in.

Fixing this is trivially easy.  (I'll post a patch in my next comment.)  But allowing plugins to query NPNVcontentsScaleFactor may change their behavior.  So we'll need to do some testing to ensure it doesn't have unfortunate side effects.
Assignee: nobody → smichaud
Blocks: 832498
Attached patch FixSplinter Review
Here's my fix.

A few quick tests with Flash and Silverlight plugins didn't turn up any problems.

And in fact this patch fixes bug 832498.
Attachment #705516 - Flags: review?(bgirard)
Attachment #705516 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/mozilla-central/rev/9a87b974b3c2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Go ahead and nominate this for uplift to mozilla-beta (it's already on Aurora now since we merged this morning) and we can get it into an early beta if it's low risk.
Comment on attachment 705516 [details] [diff] [review]
Fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 785667
User impact if declined: annoying problem with Flash, and maybe also with other plugins
Testing completed (on m-c, etc.): one month's testing on m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Given that this patch has been on m-c for a month with no complaints, it's low risk for major plugins.  Other (non-major) plugins are much more likely to be tested on the aurora and beta branches than on m-c.  So the sooner this gets onto those branches, the less likely it will cause problems in a release.

It's now the beginning of a release cycle, so the patch has already automatically landed on the aurora branch at the earliest possible point in that cycle.  Let's also get it onto the beta branch, to give it even wider testing.
Attachment #705516 - Flags: approval-mozilla-beta?
Comment on attachment 705516 [details] [diff] [review]
Fix

Low risk, already been on central a while so let's go ahead and land this now to get early beta coverage too.
Attachment #705516 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Confirmed working for me in FF 20 beta. So what's next? Will this fix be pushed to the release of FF 20 or will be applied in one of the incremental updates to FF 19?
This will be fixed in FF 20.
can we get this into a point release?

where are the tests so this never happens again?
No, we're not putting this in a point release.

But yeah smichaud, why doesn't this have a mochitest?
Flags: in-testsuite?
There's no point in a mochitest.  The original bug was a simple oversight, like a missing null check.  Do we make mochitests for missing null checks? :-)

We already have too many mochitests (and too many problems dealing with intermittent failures in them).  We should only add more for good reasons.
And as for the Flash problem (bug 832498), we *can't* write a mochitest -- since it's specific to Flash.
Yes, we do make mochitests for missing null checks. And testing this should be as simple as having the testplugin ask for the property and verify that it receives an answer. Intermittent failures are rarely a problem for simple functional tests.
> Yes, we do make mochitests for missing null checks.

I can't believe we do this as a general rule.  And if we do it's a real mistake -- a real bloat factory.

But I suppose it does make sense to make sure this property is readable.  And there is a (slim) chance that some other change might make it unreadable again -- which would be caught by a mochitest.

I'll write one later today or sometime next week.
By the way, does the test plugin run out-of-process?  If it doesn't we have no way of testing this particular bug (since it only effected out-of-process plugins).

Though I suppose I could still write a mochitest to make sure this property is readable by an in-process plugin.
The mochitests in dom/plugins/test are run both ways for full test coverage.
i don't believe we can't get a test written.  Can we get a litmus test written and have some qa person manually test it.
ignore me - thanks bsmedberg - carry on.
Attachment #727391 - Flags: review?(benjamin) → review+
Comment on attachment 727391 [details] [diff] [review]
Mochitest that queries NPNVcontentsScaleFactor

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c12a7b9d6ab6
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.