nsFormHistory.js uses global Private Browsing state to make decisions

VERIFIED FIXED in mozilla19

Status

()

defect
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

unspecified
mozilla19
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

8 years ago
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.
Posted 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 2

7 years ago
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)
Assignee

Comment 3

7 years ago
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
Assignee

Updated

7 years ago
Blocks: mobilepb
Assignee

Comment 6

7 years ago
Turns out there are not many callers! \o/
Attachment #665079 - Flags: review?(dolske)
Assignee

Updated

7 years ago
Attachment #633690 - Attachment is obsolete: true
Assignee

Comment 7

7 years ago
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

Updated

7 years ago
Assignee: nobody → josh
Assignee

Comment 10

7 years ago
Now with a test!
Attachment #668130 - Flags: review?(dolske)
Assignee

Updated

7 years ago
Attachment #665079 - Attachment is obsolete: true
Attachment #665079 - Flags: review?(dolske)
Assignee

Comment 11

7 years ago
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)
Assignee

Updated

7 years ago
Blocks: 799945
Assignee

Updated

7 years ago
Depends on: 799985
Assignee

Updated

7 years ago
No longer blocks: 799945
Assignee

Updated

7 years ago
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+

Comment 13

7 years ago
(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.
Assignee

Updated

7 years ago
Depends on: 800669

Updated

7 years ago
Blocks: 801049
Assignee

Comment 16

7 years ago
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: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Duplicate of this bug: 799985

Comment 19

7 years ago
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.