472396 - Private browsing is saving form data in certain cases (e.g. Gmail subject in compose)

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect, P1)

Description

[Noah (NoahSUMO/SolidSnake) [:Noah]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081125 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081125 Firefox/3.0

Private browsing for some unexplained reason seems to be saving form data in select cases. I can see it in Gmail's subject field when composing then sending an email. There may be more forms where this happens but I'm unsure at the moment.

Checking the formhistory.sqlite with a utility afterward, the subject form entry in the db is retained.

Also I was wondering where does the cache, history, and form history get written to while in the mode? In sqlite temp tables then dumped at end of session? Because I thought it would hard only to write to memory for the entire duration of private browsing, since its causes very slow surfing when the cache memory pref is turned off by comparison. I know cookies are saved [I think in the original cookies.sqlite] but dumped after the session. And downloads are saved only in memory according to sdwilsh.

Reproducible: Always

Steps to Reproduce:
1. Compose an email in Gmail (haven't tried other webmails)
2. Include a subject then send
3. Close and reopen Firefox, then compose another message and check subject field's form history dropdown

Comment 1

[Justin Dolske [:Dolske]]

I see this on current trunk.

1) Turn on PB
2) enter some data on a page with forms --> not saved
3) log in to Gmail, enter a subject and send --> saved
4) enter some data on a page with forms --> not saved

I added a quick printf to the top of nsFormHistory::AddEntry. It would seem PBS is reporting that it's not in private browsing mode, as inPrivateBrowsing is false.

Comment 2

[Justin Dolske [:Dolske]]

(inPrivateBrowsing is false when AddEntry is called during my step 3, I meant to say. It's correctly set to |true| during my step 2 and 4.)

Comment 3

[Marcia Knous [:marcia]]

Thanks for catching this bug Noah. Am nominating for in-litmus for now but perhaps there is a way to test for this in an automated fashion.

Comment 4

[(no longer active)]

It seems that calls to nsIPrivateBrowsingService::GetPrivateBrowsingEnabled don't make it to the service implementation (cached somewhere along the way?!)

Comment 5

[(no longer active)]

nsIPrivateBrowsingService::GetPrivateBrowsingEnabled is failing with nsresult 0x80570021.

Comment 6

[(no longer active)]

Attachment: What I used for testing

The error code returned is NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS.  In order to get more details from the nsIScriptError, I used this patch.  Here is the output for a single run which I did the following:

1. Enter PB.
2. Log in to gmail.
3. Compose a mail.

ctor
init
test
pbs
00000000
val: 1
test
pbs
80570021
[Exception... "'[JavaScript Error: "Permission denied for <https://mail.google.com> to get property Object.privateBrowsingEnabled from <>." {file: "https://mail.google.com/mail/?ui=2&view=js&name=js&ver=a9SCC6X1Wbg&am=h_mS48j2bAELJf085bO5nA" line: 551}]' when calling method: [nsIPrivateBrowsingService::privateBrowsingEnabled]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: https://mail.google.com/mail/?ui=2&view=js&name=js&ver=a9SCC6X1Wbg&am=h_mS48j2bAELJf085bO5nA :: $Rv :: line 551"  data: yes]
[JavaScript Error: "Permission denied for <https://mail.google.com> to get property Object.privateBrowsingEnabled from <>." {file: "https://mail.google.com/mail/?ui=2&view=js&name=js&ver=a9SCC6X1Wbg&am=h_mS48j2bAELJf085bO5nA" line: 551}]
val: 0

(The first successful call is made when filling in the login form.)

Here is line 551 of the mentioned file, which looks like gibberish:

l=h[Fh](Lh);for(var f=0,g=j[i];f<g;f++)rn(l[f],j[f][dr]);var o=this.rg[Fh](Cg),u=h[Fh](Cg);for(var f=0,g=o[i];f<g;f++)if(o[f][Od]==Yq)if(o[f][M]!=u[f][M]){ya(this.rg,b);h=this.rg;break}try{this.UEa=false;h.submit();c[cj]();if(Hf)this.teb=Ef(this.kMb,250,this)}catch(a){Ld(c[oe](b),Qf,this.tra,false,this);c[cj]();this.lk(2)}}};var Bwa="about:blank";

I still am not sure what gmail does which causes nsFormHistory::AddEntry to be executed in its own context.

Comment 7

[(no longer active)]

Moving to Core::XPConnect in hopes of some XPConnect guru taking a look at this...

Comment 8

[(no longer active)]

Component switch nuked the blocking flag request...

Comment 9

[Justin Dolske [:Dolske]]

(In reply to comment #6)

> Here is line 551 of the mentioned file, which looks like gibberish:
> 
> ... h.submit(); ...
> 
> I still am not sure what gmail does which causes nsFormHistory::AddEntry to be
> executed in its own context.

If the gmail code is hitting that, and it's a <form>.submit() as it would appear... It's invoking a form submission, and is going to be triggering  nsIFormSubmitObserver::Notify in nsStorageFormHistory, which in turn is calling AddEntry() to remember the form fields.

Comment 10

[(no longer active)]

(In reply to comment #9)
> If the gmail code is hitting that, and it's a <form>.submit() as it would
> appear... It's invoking a form submission, and is going to be triggering 
> nsIFormSubmitObserver::Notify in nsStorageFormHistory, which in turn is calling
> AddEntry() to remember the form fields.

Yeah, but should it cause this code to be executed with the privileges of the page itself?

As an aside, there is more to this bug than a simple .submit(), since I can't reproduce this in a reduced test case with a simple .submit() call on a form element.

Comment 11

[Noah (NoahSUMO/SolidSnake) [:Noah]]

I found another testcase if it's any help. On http://www.pizzahut.com/. All information entered from the name, address, zip code, phone number and birth date fields are all saved.

Visit https://quikorder.pizzahut.com/phorders2/login.php. Under 'Returning customers', enter any bogus username followed by a bogus password and try to login [remember my username does not have to be ticked]. Login attempt will fail but bogus username will be saved. If you follow the 'New Customers' route [on the left], interestingly the address nor zip will be saved. Once in, pick a specialty pizza (e.g. The Natural) using the 'Add to order' link underneath it. Choose a crust, scroll to the very bottom, hit 'Add to my current order'. On the right column will now be a 'My current order' frame, hit 'Check Out'. You will now have the page that saves all info mentioned in 1st paragraph.

Also this link to Deep Freeze software, http://www.faronics.com/html/calculator.asp?id=579219692&region=USA&ver=2&ship= , will save the quantity fields. I think you need to toggle the price category before the save takes place, maybe not.

Comment 12

[(no longer active)]

Attachment: Test case

Apparently I was wrong before.  In order to reproduce this, you only need to call the submit() method on a form object.

In order to reproduce this, load this test case, and enter something in the input field, and hit Enter.

Comment 13

[(no longer active)]

Attachment: Mochitest

This mochitest fails now of course, but it will pass once this bug is fixed.

Comment 14

[(no longer active)]

Could this be because of the fact that form submissions which are a result of clicking the submit button are deferred <http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLFormElement.cpp#1564>, and those that are invoked from scripts are not?

CCing some folks which may be able to provide insight here.

Comment 15

[Justin Dolske [:Dolske]]

Comment on attachment 355925 [details] [diff] [review]
Mochitest

>--- a/toolkit/components/satchel/test/Makefile.in
...
>+MOCHI_TESTS = \
>+		test_bug_472396.html \

Let's call this something like test_privbrowsing.html. Ditto for the subtest.

>+++ b/toolkit/components/satchel/test/satchel_common.js
...
>+function cleanUpFormHist() {

This should just do a getService() for nsIFormHistory2 and call removeAllEntries().

>+++ b/toolkit/components/satchel/test/subtst_bug_472396.html
...
>+  <form id="form">
>+    <input name="username" id="username">
>+  </form>

Use something other than "username", so there's no confusion with thinking the password manager might be involved with this.

Hmm, speaking of password manager, it's interesting that this bug doesn't seem to affect it (at least not the current mochitests it has for this). Once the cause is understood, we should look to see if any extra tests are needed.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(cc'ing jst to double-check that i'm saying the right thing since my head is
all clogged up with having a cold).

If there is JS on the stack we'll use the security principal of that JS when
calling into a JS implemented XPCOM component.

There's a few ways of fixing this:

1. Flag private browsing XPCOM component as safe to call for anyone. There's a
couple of ways of doing this, either by using nsIClassInfo, or
nsISecurityCheckedComponent.
2. Implement the private browsing component in C++ instead.
3. Push a null JS context before calling into the private browsing component.
This has to be done at every callsite.

3 seems like a bad idea as that'll be hard to get right everywhere. Don't know
how much work 2 would be. 1 seems the simplest, but has the downside that if
content JS is ever able to get a reference to the private browsing component it
could call any function on it.

Comment 17

[(no longer active)]

Attachment: Mochitest (v2)

Comments addressed.  I made the failing test a todo so that we can land the test before the fix.

Comment 18

[(no longer active)]

(In reply to comment #16)
> If there is JS on the stack we'll use the security principal of that JS when
> calling into a JS implemented XPCOM component.

Hmmm, has this never caused any other problems in the past?  It seems a bit weird; I'm not sure what the reasoning behind this is.

> There's a few ways of fixing this:
> 
> 1. Flag private browsing XPCOM component as safe to call for anyone. There's a
> couple of ways of doing this, either by using nsIClassInfo, or
> nsISecurityCheckedComponent.

Hmmm, I couldn't find any relevant flags in <https://developer.mozilla.org/en/NsIClassInfo/flags>.  <http://mxr.mozilla.org/mozilla-central/ident?i=nsISecurityCheckedComponent> doesn't show any other JS components doing this as well.

Also, what other components already do this?  Daniel, what are the security implications here?  It's a bad thing for web content to be able to change (or even read) the private browsing mode status...  But I can't think of any way that we would allow web content to get a hold of a reference to the PB service as well (although a badly written extension might).

> 2. Implement the private browsing component in C++ instead.

We can do this as a last resort, but I prefer other solutions if possible because of the safety properties of the JavaScript language.  Rewriting the whole component in C++ is possible, albeit not pleasant.

> 3. Push a null JS context before calling into the private browsing component.
> This has to be done at every callsite.

Only for C++ callers, right?  And only for C++ callers which may end up being called from web content script code, right?  If that's the case, then it may be a viable solution, if not a perfect one.  If we decide to take this route, maybe we can provide a small helper header which defines a helper function which callers can use.

> 3 seems like a bad idea as that'll be hard to get right everywhere. Don't know
> how much work 2 would be. 1 seems the simplest, but has the downside that if
> content JS is ever able to get a reference to the private browsing component it
> could call any function on it.

This is especially dangerous because of removeDataFromDomain method (which I would argue that it should not have been part of the nsIPrivateBrowsingService interface in the first place).

Comment 19

[(no longer active)]

One other thought (possibly stupid): inside the implementation of nsISecurityCheckedComponent (in a JS component), is there any way to detect if we're being called directly from web content script code, or with a C++ intermediate layer?  If there is, then maybe we can restrict access to the object if it's being directly called from the web?

Comment 20

[(no longer active)]

(In reply to comment #16)
> (cc'ing jst to double-check that i'm saying the right thing since my head is
> all clogged up with having a cold).

You forgot to CC jst by the way.  :-)

jst: please see comment 16.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > There's a few ways of fixing this:
> > 
> > 1. Flag private browsing XPCOM component as safe to call for anyone. There's a
> > couple of ways of doing this, either by using nsIClassInfo, or
> > nsISecurityCheckedComponent.
> 
> Hmmm, I couldn't find any relevant flags in
> <https://developer.mozilla.org/en/NsIClassInfo/flags>. 
> <http://mxr.mozilla.org/mozilla-central/ident?i=nsISecurityCheckedComponent>
> doesn't show any other JS components doing this as well.

I believe it should work even though it's not currently done in gecko code.

> Also, what other components already do this?  Daniel, what are the security
> implications here?  It's a bad thing for web content to be able to change (or
> even read) the private browsing mode status...  But I can't think of any way
> that we would allow web content to get a hold of a reference to the PB service
> as well (although a badly written extension might).

It sounds very scary to me if web pages can enter and exit private mode.

I think it should be ok to flag it as safe though since there really should be no way for web content to get hold if this component.

> > 3. Push a null JS context before calling into the private browsing component.
> > This has to be done at every callsite.
> 
> Only for C++ callers, right?  And only for C++ callers which may end up being
> called from web content script code, right?  If that's the case, then it may be
> a viable solution, if not a perfect one.  If we decide to take this route,
> maybe we can provide a small helper header which defines a helper function
> which callers can use.

I would strongly advice against this solution. It's always bad to have a situation where forgetting to do something is both easy to do, and results in security and/or privacy problems.

> > 3 seems like a bad idea as that'll be hard to get right everywhere. Don't know
> > how much work 2 would be. 1 seems the simplest, but has the downside that if
> > content JS is ever able to get a reference to the private browsing component it
> > could call any function on it.
> 
> This is especially dangerous because of removeDataFromDomain method (which I
> would argue that it should not have been part of the nsIPrivateBrowsingService
> interface in the first place).

Indeed.

Comment 22

[Boris Zbarsky [:bzbarsky]]

It seems like using nsISecurityCheckedComponent and white-listing the things that should be readable in this case is the way to go...  (but NOT whitelisting canCreateWrapper, etc).

Comment 23

[Bartłomiej Brzozowiec (BartZilla)]

Just nitpicking...
(From update of attachment 355926 [details] [diff] [review])
>+      "The username entry should not be saved in private browsing mode");

>+  ok(!formhist.entryExists("field", "value"), "A username entry should not exist at startup");

There is no "username" field anymore, so messages should be changed, I guess...

Comment 24

[Johnny Stenback (:jst)]

Yum, nsISecurityCheckedComponent does sound like the way to go here. Back to Private Browsing.

Comment 25

[(no longer active)]

Attachment: First attempt

OK, here is my first try.  This is all new territory to me, so I'm not sure what I'm doing wrong.  When running the unit test, the only nsISecurityCheckedComponent method being called is canSetProperty with the privateBrowsingEnabled property (twice).  This is because the unit test itself sets this property twice.

It seems to me that nsISecurityCheckedComponent is not checked at all when a C++ method calls a JS method; it's only checked when a JS method calls a JS method directly.  If that is the case, then nsISecurityCheckedComponent won't be useful here.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It's only checked when a JS method calls a JS component *indirectly* through XPConnect. In this case you're calling through C++, which obviously goes through XPConnect, but it'll also be called if JS code gets a reference to your component through XPCOM since all calls will then go through JS->XPConnect->JS

Comment 27

[(no longer active)]

So it won't be useful here, right?  What we have here is like this:

[unprivileged JS code] -> C++ -> XPConnect -> [JS component code]

Comment 28

[Boris Zbarsky [:bzbarsky]]

Hmm.  So do we possibly have a stack to where the exception is being thrown?  Looking at that code should put to rest questions about whether nsISecurityCheckedComponent should work in this situation...

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #27)
> [unprivileged JS code] -> C++ -> XPConnect -> [JS component code]

Any time we are potentially restricting access to a JS component we also check with the nsISecurityCheckedComponent interface. So either we do no checks and always allow, or we check, and then nsISecurityCheckedComponent will tell us that we should allow. In both cases you are fine.

XPConnect is what does the checking, and it does it anytime you have JS code higher up on the callstack. Those two conditions are true in the call-chain you are showing.

Comment 30

[(no longer active)]

(In reply to comment #28)
> Hmm.  So do we possibly have a stack to where the exception is being thrown? 
> Looking at that code should put to rest questions about whether
> nsISecurityCheckedComponent should work in this situation...

Here is the stack trace taken from the call to nsIPrivateBrowsingService::GetPrivateBrowsingEnabled():

 	satchel.dll!nsFormHistory::AddEntry(const nsAString_internal & aName={...}, const nsAString_internal & aValue={...})  Line 261	C++
 	satchel.dll!nsFormHistory::Notify(nsIDOMHTMLFormElement * formElt=0x07328910, nsIDOMWindowInternal * aWindow=0x040f9568, nsIURI * aActionURL=0x06b154a8, int * aCancelSubmit=0x0027db90)  Line 507	C++
 	gklayout.dll!nsHTMLFormElement::NotifySubmitObservers(nsIURI * aActionURL=0x06b154a8, int * aCancelSubmit=0x0027db90, int aEarlyNotify=1)  Line 1199 + 0x46 bytes	C++
 	gklayout.dll!nsHTMLFormElement::SubmitSubmission(nsIFormSubmission * aFormSubmission=0x069cf308)  Line 1094 + 0x1d bytes	C++
 	gklayout.dll!nsHTMLFormElement::DoSubmit(nsEvent * aEvent=0x00000000)  Line 1007 + 0x11 bytes	C++
 	gklayout.dll!nsHTMLFormElement::DoSubmitOrReset(nsEvent * aEvent=0x00000000, int aMessage=1200)  Line 932 + 0xc bytes	C++
 	gklayout.dll!nsHTMLFormElement::Submit()  Line 667 + 0x12 bytes	C++
 	xpc3250.dll!nsIDOMHTMLFormElement_Submit(JSContext * cx=0x069a2ac0, unsigned int argc=0, long * vp=0x067fc95c)  Line 6636 + 0x11 bytes	C++
 	js3250.dll!js_Interpret(JSContext * cx=0x069a2ac0)  Line 4994 + 0x17 bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x069a2ac0, unsigned int argc=1, long * vp=0x067fc8cc, unsigned int flags=0)  Line 1336 + 0x9 bytes	C++
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x069a2ac0, JSObject * obj=0x048523c0, long fval=75836928, unsigned int flags=0, unsigned int argc=1, long * argv=0x067fc8c8, long * rval=0x0027e514)  Line 1393 + 0x15 bytes	C++
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x069a2ac0, JSObject * obj=0x048523c0, long fval=75836928, unsigned int argc=1, long * argv=0x067fc8c8, long * rval=0x0027e514)  Line 5247 + 0x1f bytes	C++
 	gklayout.dll!nsJSContext::CallEventHandler(nsISupports * aTarget=0x040f9770, void * aScope=0x048523c0, void * aHandler=0x04852e00, nsIArray * aargv=0x06df0d40, nsIVariant * * arv=0x0027e6e0)  Line 1998 + 0x21 bytes	C++
 	gklayout.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent=0x06e77da8)  Line 247 + 0x67 bytes	C++
 	gklayout.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct * aListenerStruct=0x04181c38, nsIDOMEventListener * aListener=0x06e6b3c8, nsIDOMEvent * aDOMEvent=0x06e77da8, nsPIDOMEventTarget * aCurrentTarget=0x040f95a8, unsigned int aPhaseFlags=6)  Line 1090 + 0x12 bytes	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext * aPresContext=0x0714f688, nsEvent * aEvent=0x0027eaac, nsIDOMEvent * * aDOMEvent=0x0027e9e8, nsPIDOMEventTarget * aCurrentTarget=0x040f95a8, unsigned int aFlags=6, nsEventStatus * aEventStatus=0x0027e9ec)  Line 1197	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6, int aMayHaveNewListenerManagers=1)  Line 228	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6, nsDispatchingCallback * aCallback=0x00000000, int aMayHaveNewListenerManagers=1)  Line 293	C++
 	gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x040f9568, nsPresContext * aPresContext=0x0714f688, nsEvent * aEvent=0x0027eaac, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0027eaa8, nsDispatchingCallback * aCallback=0x00000000)  Line 508 + 0x14 bytes	C++
 	gklayout.dll!DocumentViewerImpl::LoadComplete(unsigned int aStatus=0)  Line 990 + 0x21 bytes	C++
 	docshell.dll!nsDocShell::EndPageLoad(nsIWebProgress * aProgress=0x067b90ec, nsIChannel * aChannel=0x04135e48, unsigned int aStatus=0)  Line 5243	C++
 	docshell.dll!nsWebShell::EndPageLoad(nsIWebProgress * aProgress=0x067b90ec, nsIChannel * channel=0x04135e48, unsigned int aStatus=0)  Line 1018	C++
 	docshell.dll!nsDocShell::OnStateChange(nsIWebProgress * aProgress=0x067b90ec, nsIRequest * aRequest=0x04135e48, unsigned int aStateFlags=131088, unsigned int aStatus=0)  Line 5139	C++
 	docshell.dll!nsDocLoader::FireOnStateChange(nsIWebProgress * aProgress=0x067b90ec, nsIRequest * aRequest=0x04135e48, int aStateFlags=131088, unsigned int aStatus=0)  Line 1236	C++
 	docshell.dll!nsDocLoader::doStopDocumentLoad(nsIRequest * request=0x04135e48, unsigned int aStatus=0)  Line 869	C++
 	docshell.dll!nsDocLoader::DocLoaderIsEmpty()  Line 765	C++
 	docshell.dll!nsDocLoader::OnStopRequest(nsIRequest * aRequest=0x0714eaa8, nsISupports * aCtxt=0x00000000, unsigned int aStatus=0)  Line 682	C++
 	necko.dll!nsLoadGroup::RemoveRequest(nsIRequest * request=0x0714eaa8, nsISupports * ctxt=0x00000000, unsigned int aStatus=0)  Line 688 + 0x2e bytes	C++
 	gklayout.dll!nsDocument::DoUnblockOnload()  Line 7065	C++
 	gklayout.dll!nsDocument::UnblockOnload(int aFireSync=1)  Line 7011	C++
 	gklayout.dll!nsDocument::DispatchContentLoadedEvents()  Line 3974	C++
 	gklayout.dll!nsRunnableMethod<nsDocument>::Run()  Line 265	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0027f354)  Line 511	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x011628c8, int mayWait=1)  Line 230 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 170 + 0xc bytes	C++
 	toolkitcomps.dll!nsAppStartup::Run()  Line 192 + 0x1c bytes	C++
 	xul.dll!XRE_main(int argc=5, char * * argv=0x0116bfd8, const nsXREAppData * aAppData=0x00fc0880)  Line 3210 + 0x25 bytes	C++
 	firefox.exe!NS_internal_main(int argc=5, char * * argv=0x0116bfd8)  Line 156 + 0x12 bytes	C++
 	firefox.exe!wmain(int argc=5, wchar_t * * argv=0x0116e4d8)  Line 87 + 0xd bytes	C++
 	firefox.exe!__tmainCRTStartup()  Line 579 + 0x19 bytes	C
 	firefox.exe!wmainCRTStartup()  Line 399	C
 	kernel32.dll!76883833() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	
 	ntdll.dll!77c8a9bd() 	

I tried stepping through it, but it would corrupt the stack trace.

I think based on this stack trace, the description I gave in comment 27 should be accurate, but I may be wrong.  Anyone has any ideas?

Comment 31

[(no longer active)]

I went further down the stack, and here is where the security decision seems to be made: <http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#674>, setting the security level to SCRIPT_SECURITY_SAME_ORIGIN_ACCESS, and later on nsScriptSecurityManager::CheckSameOriginDOMProp would deny the access <http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#723>.

Later on, when CheckPropertyAccessImpl tries to QI for nsISecurityCheckedComponent, it does it on aObj <http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#767>, which is null in this call.  This is why the nsISecurityCheckedComponent doesn't solve anything.

Here's the stack trace of this point in execution:

	caps.dll!nsScriptSecurityManager::CheckPropertyAccessImpl(unsigned int aAction=1, nsAXPCNativeCallContext * aCallContext=0x00000000, JSContext * cx=0x069a1638, JSObject * aJSObject=0x02101540, nsISupports * aObj=0x00000000, nsIURI * aTargetURI=0x00000000, nsIClassInfo * aClassInfo=0x00000000, const char * aClassName=0x6c8663cc, long aProperty=33504156, void * * aCachedClassPolicy=0x00000000)  Line 767	C++
 	caps.dll!nsScriptSecurityManager::CheckPropertyAccess(JSContext * cx=0x069a1638, JSObject * aJSObject=0x02101540, const char * aClassName=0x6c8663cc, long aProperty=33504156, unsigned int aAction=1)  Line 534	C++
 	caps.dll!nsScriptSecurityManager::CheckObjectAccess(JSContext * cx=0x069a1638, JSObject * obj=0x02842340, long id=33504156, JSAccessMode mode=JSACC_READ, long * vp=0x0027d0bc)  Line 516 + 0x36 bytes	C++
 	js3250.dll!js_InternalGetOrSet(JSContext * cx=0x069a1638, JSObject * obj=0x02842340, long id=33504156, long fval=34608448, JSAccessMode mode=JSACC_READ, unsigned int argc=0, long * argv=0x00000000, long * rval=0x0027d330)  Line 1450 + 0x15b bytes	C++
 	js3250.dll!js_NativeGet(JSContext * cx=0x069a1638, JSObject * obj=0x02842340, JSObject * pobj=0x02130e80, JSScopeProperty * sprop=0x0317be40, long * vp=0x0027d330)  Line 3732 + 0x30 bytes	C++
 	js3250.dll!js_GetPropertyHelper(JSContext * cx=0x069a1638, JSObject * obj=0x02842340, long id=33504156, long * vp=0x0027d330, JSPropCacheEntry * * entryp=0x00000000)  Line 3883 + 0x19 bytes	C++
 	js3250.dll!js_GetProperty(JSContext * cx=0x069a1638, JSObject * obj=0x02842340, long id=33504156, long * vp=0x0027d330)  Line 3897 + 0x17 bytes	C++
 	js3250.dll!JS_GetProperty(JSContext * cx=0x069a1638, JSObject * obj=0x02842340, const char * name=0x01d8aec8, long * vp=0x0027d330)  Line 3623 + 0x41 bytes	C++
 	xpc3250.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS * wrapper=0x069e2ca8, unsigned short methodIndex=3, const XPTMethodDescriptor * info=0x01d8ae88, nsXPTCMiniVariant * nativeParams=0x0027d4ac)  Line 1599 + 0x22 bytes	C++
 	xpc3250.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex=3, const XPTMethodDescriptor * info=0x01d8ae88, nsXPTCMiniVariant * params=0x0027d4ac)  Line 564	C++
 	xpcom_core.dll!PrepareAndDispatch(nsXPTCStubBase * self=0x06fa9338, unsigned int methodIndex=3, unsigned int * args=0x0027d56c, unsigned int * stackBytesToPop=0x0027d55c)  Line 114 + 0x21 bytes	C++
 	xpcom_core.dll!SharedStub()  Line 142	C++
 	satchel.dll!nsFormHistory::AddEntry(const nsAString_internal & aName={...}, const nsAString_internal & aValue={...})  Line 262	C++
 	gklayout.dll!nsHTMLInputElement::GetName(nsAString_internal & aValue={...})  Line 728 + 0x19 bytes	C++
 	xpc3250.dll!nsXPConnect::Release()  Line 62 + 0x5b bytes	C++
 	80000002()	

After this, the code goes on to generate an error message and report it.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Oh, right.  See bug 360207.

Think you want to take on fixing it?  Or do we want to try to find a way to work around here?

Comment 33

[(no longer active)]

(In reply to comment #32)
> Oh, right.  See bug 360207.
> 
> Think you want to take on fixing it?  Or do we want to try to find a way to
> work around here?

I think I'll give fixing that bug a shot.  I may of course need some (a lot of) help there...

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

Ehsan, any update or ETA? We're trying for code freeze for Beta 3 on Sunday, and this doesn't feel like it's gonna make it.

Comment 35

[(no longer active)]

(In reply to comment #34)
> Ehsan, any update or ETA? We're trying for code freeze for Beta 3 on Sunday,
> and this doesn't feel like it's gonna make it.

Things aren't looking so good here.  This basically needs bug 360207 to be fixed, which I haven't figured out how to fix yet, and we don't have a workaround for this bug yet which we can use until bug 360207 gets fixed.  And me having caught a cold doesn't help either.  :-(  So I think there's a good chance that this won't make it for the Beta 3 code freeze (unless someone can give me a hint to get me started at least).

Comment 36

[Johnny Stenback (:jst)]

I think given where we are here wrt deadlines etc, I think (based on my limited knowledge about our private browsing code) that the two best options here are:

1) re-write the implementation of nsIPrivateBrowsingService in C++
2) write a wrapper for the JS component in C++ that pushes a null context
   onto the JS context stack before calling into the JS component to get
   around these problems.

The latter of hose is probably the easier and safer of the two. Long term it'd be great to fix the bugs causing this, of course, but I really don't think now would be the time to do that.

Comment 37

[(no longer active)]

Attachment: Patch (v1)

I ended up pushing a null js context on the context stack before the call to nsIPrivateBrowsingService::GetPrivateBrowsingEnabled, and it fixes the problem.  The unit test now passes successfully.  Requesting review from dolske.

I also prefer either jst or bz to take a look at this patch to make sure I haven't missed anything, so I'm requesting review from both to see who can get to it sooner.

Thanks!

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note that you need to push a null context at *every* call site into your component. We're bound to miss someplace otherwise.

Comment 39

[Johnny Stenback (:jst)]

Comment on attachment 358554 [details] [diff] [review]
Patch (v1)

- In nsFormHistory::AddEntry():

   // If the user is in private browsing mode, don't add any entry.
   PRBool inPrivateBrowsing = PR_FALSE;
   nsCOMPtr<nsIPrivateBrowsingService> pbs =
     do_GetService(NS_PRIVATE_BROWSING_SERVICE_CONTRACTID);
-  if (pbs)
-    pbs->GetPrivateBrowsingEnabled(&inPrivateBrowsing);
+  if (pbs) {
+    // Temporary fix for bug 472396: push a null js context on the context stack
+    // so that calls made through web page scripts succeed.
+    nsCOMPtr<nsIJSContextStack> stack =
+      do_GetService("@mozilla.org/js/xpc/ContextStack;1");
+    if (stack && NS_SUCCEEDED(stack->Push(nsnull))) {
+      pbs->GetPrivateBrowsingEnabled(&inPrivateBrowsing);
+      JSContext *cx;
+      stack->Pop(&cx);
+      NS_ASSERTION(cx == nsnull, "JSContextStack mismatch");
+    }
+  }
   if (inPrivateBrowsing)
     return NS_OK;

Looks good to me, but maybe add an error check for the GetPrivateBrowsingEnabled() return value too? And I'd prefer to see the default for inPrivateBrowsing be PR_TRUE, at least in the case when we do have a private browsing service, so that we err on the side of caution here. If any of the above fails, I'd rather not save the data than store the data when the user thinks its not being stored. But I could see if the private browsing service doesn't exist that we'd fall back to assuming we're not in private mode. To do that you could move the declaration and checking of the variable inPrivateBrowsing inside the if (pbs) check, or something.

r=jst with that considered.

Comment 40

[(no longer active)]

(In reply to comment #38)
> Note that you need to push a null context at *every* call site into your
> component. We're bound to miss someplace otherwise.

This is the only call site from satchel.  Should I do this for all other C++ call sites as well in this bug?

Comment 41

[(no longer active)]

Attachment: Patch (v1.1)

Addressed jst's review comments.

Comment 42

[(no longer active)]

I filed bug 475141 to track creating a C++ wrapper service for all C++ callers
of the private browsing service in case bug 360207 is not fixed in time for the
3.1 release.

Comment 43

[(no longer active)]

Justin: is your r+ also valid for attachment 358560 [details] [diff] [review]?

Comment 44

[Justin Dolske [:Dolske]]

Comment on attachment 358560 [details] [diff] [review]
Patch (v1.1)

Oops. Reviewed the old version while you attached the new version. Bugzilla didn't say anything! Both look fine.

Comment 45

[(no longer active)]

Fixed on m-c: <http://hg.mozilla.org/mozilla-central/rev/49800e96c9c0>

Will land on branch after a green cycle.

Comment 46

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #40)
> (In reply to comment #38)
> > Note that you need to push a null context at *every* call site into your
> > component. We're bound to miss someplace otherwise.
> 
> This is the only call site from satchel.  Should I do this for all other C++
> call sites as well in this bug?

I guess I don't care if it happens in this bug or not. Yes, you should do it from every C++ call site, just to be on the safe side.

Comment 47

[(no longer active)]

(In reply to comment #46)
> I guess I don't care if it happens in this bug or not. Yes, you should do it
> from every C++ call site, just to be on the safe side.

I filed bug 475141 for this issue.

Comment 48

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1889e9b4935f

Comment 49

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I think the right fix was to have a C++ shim that just forwards to the JS impl and does the push, so that you do in fact fix all possible C++ callers.  The patch that was checked in works for form data, but won't work for the other callers.

As in, the followup bug should, imo, block release.

Comment 50

[Marcia Knous [:marcia]]

verified fixed on the 1.9.1 branch using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090126 Shiretoko/3.1b3pre and  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090126 Shiretoko/3.1b3pre. I tested using the pizzahut site and the testcase in the attachments portion of the bug. Adding the verified keyword.