Last Comment Bug 748752 - nsNPAPIPlugin check for private browsing value will not work
: nsNPAPIPlugin check for private browsing value will not work
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: :Ehsan Akhgari (out sick)
:
Mentors:
Depends on: 749795
Blocks: PBnGen 722942
  Show dependency treegraph
 
Reported: 2012-04-25 06:57 PDT by Josh Matthews [:jdm]
Modified: 2012-04-30 10:32 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (875 bytes, patch)
2012-04-25 08:57 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Patch (v2) (922 bytes, patch)
2012-04-25 19:55 PDT, :Ehsan Akhgari (out sick)
jaas: review+
Details | Diff | Review

Description Josh Matthews [:jdm] 2012-04-25 06:57:20 PDT
>::: 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 :Ehsan Akhgari (out sick) 2012-04-25 08:57:23 PDT
Created attachment 618300 [details] [diff] [review]
Patch (v1)
Comment 2 Josh Matthews [:jdm] 2012-04-25 09:25:56 PDT
Every other caller of GetDocumentFromNPP null checks, so we should probably do the same.
Comment 3 :Ehsan Akhgari (out sick) 2012-04-25 19:55:35 PDT
Created attachment 618516 [details] [diff] [review]
Patch (v2)
Comment 4 Josh Aas 2012-04-26 07:55:48 PDT
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 :Ehsan Akhgari (out sick) 2012-04-26 08:55:30 PDT
(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 :Ehsan Akhgari (out sick) 2012-04-26 11:18:10 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf2eb02383fd
Comment 7 Josh Matthews [:jdm] 2012-04-26 22:40:21 PDT
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] 2012-04-27 07:04:22 PDT
https://hg.mozilla.org/mozilla-central/rev/bf2eb02383fd
Comment 9 :Ehsan Akhgari (out sick) 2012-04-27 14:16:42 PDT
Josh, you said in bug 749795 that you're working on a test for this, right?
Comment 10 Josh Matthews [:jdm] 2012-04-28 00:53:51 PDT
Yes.

Note You need to log in before you can comment on or make changes to this bug.