Closed Bug 723001 Opened 9 years ago Closed 8 years ago

nsFormHistory.js uses global Private Browsing state to make decisions

Categories

(Toolkit :: Form Manager, defect)

x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla19

People

(Reporter: jdm, Assigned: jdm)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch WIP(Patch v1) (obsolete) — Splinter Review
Please tell me which tests to run for this patch. Also which others parts would I need to change for this patch to work ?
Assignee: nobody → saurabhanandiit
Status: NEW → ASSIGNED
Attachment #633690 - Flags: feedback?(ehsan)
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.
Attachment #633690 - Flags: feedback?(ehsan)
Since this is toolkit code, we should probably check the nsILoadContext specifically instead of relying on the gPrivateBrowsingUI object.
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?
I'm currently working on something else, and have put this on halt. Hence unassigning myself, so that I don't block someone else.
Assignee: saurabhanandiit → nobody
Blocks: mobilepb
Turns out there are not many callers! \o/
Attachment #665079 - Flags: review?(dolske)
Attachment #633690 - Attachment is obsolete: true
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.
Assignee: nobody → josh
Now with a test!
Attachment #668130 - Flags: review?(dolske)
Attachment #665079 - Attachment is obsolete: true
Attachment #665079 - Flags: review?(dolske)
Comment on attachment 668130 [details] [diff] [review]
Move privacy state checking from nsFormHistory to its callers.

No reviewing this until tests pass.
Attachment #668130 - Flags: review?(dolske)
Blocks: 799945
Depends on: 799985
No longer blocks: 799945
Attachment #668130 - Flags: review?(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.
Attachment #668130 - Flags: review?(dolske) → review+
(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.
Depends on: 800669
Blocks: 801049
Landed https://hg.mozilla.org/integration/mozilla-inbound/rev/8fa504f1802e with the removal of test_bug_248970.js.
https://hg.mozilla.org/mozilla-central/rev/8fa504f1802e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Duplicate of this bug: 799985
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.