723001 - nsFormHistory.js uses global Private Browsing state to make decisions

Status: VERIFIED FIXED

(Toolkit :: Form Manager, defect)

Description

[Josh Matthews [:jdm]]

The global state is going away. We probably should just update callers of FormHistory.addEntry to check the window's docshell, because there doesn't look like a good place to do it inside FormHistory.

Comment 1

[Saurabh Anand [:sawrubh]]

Attachment: WIP(Patch v1)

Please tell me which tests to run for this patch. Also which others parts would I need to change for this patch to work ?

Comment 2

[(no longer active)]

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

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

So I actually think that this is not the correct way to fix this bug.  The window object doesn't exist in this component, as the code is shared between all browser windows.  This is the sort of thing which we should probably fix in the callers of addEntry.  One of the callers is here, for example: <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/formSubmitListener.js#96>, and here we do have access to the window object, so we can return early if the window is in PB mode.

You should check to see whether there are other callers to fix as well.

Currently we do have a test which tests this feature: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/test/unit/test_bug_248970.js

But since that uses the addEntry API directly, you can't use this test if you fix this in the callers.  In this case I think you should use one of the existing tests, such as test_privbrowsing.html in the same directory, and I think you should be able to use the same test here as well.

Comment 3

[Josh Matthews [:jdm]]

Since this is toolkit code, we should probably check the nsILoadContext specifically instead of relying on the gPrivateBrowsingUI object.

Comment 4

[Justin Dolske [:Dolske]]

Yeah, I'd suggest adding a check to notify() in formSubmitListener.js. At the very least, that would make the by-far most common case short-circuit and simply not attempt to store anything.

The only other caller I can think of offhand would be entries from the search box (next to the URL bar), or more generally (I think) any XUL autocomplete input.

Those two may be sufficient; the belt-and-suspenders approach would be to also add a hopefully-redundant check to the actual storage module (as you've done), but maybe that has risk of catching non-private callers when it shouldn't?

Comment 5

[Saurabh Anand [:sawrubh]]

I'm currently working on something else, and have put this on halt. Hence unassigning myself, so that I don't block someone else.

Comment 6

[Josh Matthews [:jdm]]

Attachment: Move privacy state checking from nsFormHistory to its callers.

Turns out there are not many callers! \o/

Comment 7

[Josh Matthews [:jdm]]

Brian, assuming my searches were correct, as long as Fennec is just using the form submit listener code there shouldn't be any changes required on your end.

Comment 8

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=eaf2d89d1f8f

Comment 9

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=797c56ce5cd7

Comment 10

[Josh Matthews [:jdm]]

Attachment: Move privacy state checking from nsFormHistory to its callers.

Now with a test!

Comment 11

[Josh Matthews [:jdm]]

Comment on attachment 668130 [details] [diff] [review]
Move privacy state checking from nsFormHistory to its callers.

No reviewing this until tests pass.

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 668130 [details] [diff] [review]
Move privacy state checking from nsFormHistory to its callers.

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

r+ assuming test_privbrowsing.html and test_bug_248970.js are still relevant in the grand new world.

Comment 13

[(no longer active)]

(In reply to comment #12)
> r+ assuming test_privbrowsing.html and test_bug_248970.js are still relevant in
> the grand new world.

They need to be rewritten at some point.  But they're testing the right thing.

Comment 14

[Josh Matthews [:jdm]]

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

Comment 15

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6db2ba78beb8

Comment 16

[Josh Matthews [:jdm]]

Landed https://hg.mozilla.org/integration/mozilla-inbound/rev/8fa504f1802e with the removal of test_bug_248970.js.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8fa504f1802e

Comment 19

[Andreea Pod]

Verified fixed following the steps from Bug 799985 
- build: Firefox for Android 20.0a1(2012-12-18)
- device: Samsung Galaxy Nexus 
- OS: Android 4.1.2