748752 - nsNPAPIPlugin check for private browsing value will not work

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Matthews [:jdm]]

>::: dom/plugins/base/nsNPAPIPlugin.cpp
>@@ +2136,5 @@
>>    }
>>  
>>    case NPNVprivateModeBool: {
>> +    nsCOMPtr<nsIDocument> doc = GetDocumentFromNPP(npp);
>> +    nsCOMPtr<nsPIDOMWindow> domwindow = do_QueryInterface(doc);
>
>This will always be null.

Looks like this should be |domwindow = doc->GetWindow()| instead.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Josh Matthews [:jdm]]

Every other caller of GetDocumentFromNPP null checks, so we should probably do the same.

Comment 3

[(no longer active)]

Attachment: Patch (v2)

Comment 4

[Josh Aas]

Comment on attachment 618516 [details] [diff] [review]
Patch (v2)

Review of attachment 618516 [details] [diff] [review]:
-----------------------------------------------------------------

Is this something a test should have caught? We do have an NPAPI private mode test.

dom/plugins/test/mochitest/test_privatemode.xul

Comment 5

[(no longer active)]

(In reply to Josh Aas (Mozilla Corporation) from comment #4)
> Comment on attachment 618516 [details] [diff] [review]
> Patch (v2)
> 
> Review of attachment 618516 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this something a test should have caught? We do have an NPAPI private
> mode test.
> 
> dom/plugins/test/mochitest/test_privatemode.xul

I have no idea how to trigger this code in a test...

Comment 6

[(no longer active)]

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

Comment 7

[Josh Matthews [:jdm]]

The problem existed in the NPN_GetValue code, which is tested by queryPrivateModeState (http://mxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_privatemode.xul#38). The test as written would pass, since it tests non-PB with qPMS, then tests that entering PB mode triggers an update message for plugin instances via NPN_SetValue (http://mxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_privatemode.xul#59). Since it doesn't compare this against the qPMS value, the test doesn't see anything as being wrong. This check should be added to the test, I think.

Comment 8

[Ed Morley [:emorley]]

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

Comment 9

[(no longer active)]

Josh, you said in bug 749795 that you're working on a test for this, right?

Comment 10

[Josh Matthews [:jdm]]

Yes.