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

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

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.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 705516 [details] [diff] [review]
Fix

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a87b974b3c2

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a87b974b3c2

Comment 4

[Georg Fritzsche [:gfritzsche]]

Nominating for tracking per bug 832498, comment 1 and bug 832498, comment 4.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

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 6

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 705516 [details] [diff] [review]
Fix

Landed on mozilla-beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/9b2eda41c48d

Comment 9

[kai distill]

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?

Comment 10

[Steven Michaud [:smichaud] (Retired)]

This will be fixed in FF 20.

Comment 11

[Doug Turner (:dougt)]

can we get this into a point release?

where are the tests so this never happens again?

Comment 12

[Benjamin Smedberg]

No, we're not putting this in a point release.

But yeah smichaud, why doesn't this have a mochitest?

Comment 13

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

And as for the Flash problem (bug 832498), we *can't* write a mochitest -- since it's specific to Flash.

Comment 15

[Benjamin Smedberg]

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.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

> 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.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 18

[Benjamin Smedberg]

The mochitests in dom/plugins/test are run both ways for full test coverage.

Comment 19

[Doug Turner (:dougt)]

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.

Comment 20

[Doug Turner (:dougt)]

ignore me - thanks bsmedberg - carry on.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Attachment: Mochitest that queries NPNVcontentsScaleFactor

Comment 22

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 727391 [details] [diff] [review]
Mochitest that queries NPNVcontentsScaleFactor

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c12a7b9d6ab6

Comment 23

[Ed Morley [:emorley]]

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