The Open Web Location dialog persists the last URL or search terms you enter across changes to the status of private browsing, which means it can reveal private URLs or search terms that you enter while browsing privately and don't want to reveal to others after you leave that mode. Steps to Reproduce: 1. enter private browsing mode; 2. open the Open Web Location dialog;* 3. enter anything into the text field and press the Open button; 4. exit private browsing mode; 5. open the Open Web Location dialog again. * There are (at least) two ways to open the Open Web Location dialog. One is to be in full screen mode on Windows or Linux (View > Full Screen). The other is to remove the location bar from the navigation toolbar on all OSes using View > Toolbars > Customize... In either case, selecting File > Open Location will then open the Open Web Location dialog instead of focusing the location bar. Expected Results: the text you entered into the text field is no longer there. Actual Results: the text you entered into the text field is still there. This seems significant enough to warrant some status wrt 3.5. Requesting blocking so drivers see it and can determine whether it warrants blocking or wanted status.

Indeed, the code in openLocation.js needs to avoid setting a pref in private browsing mode. (We made "Clear private data..." clear this data in bug 397196.)

Confirmed in latest 1.9.2 trunk Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090620 Minefield/3.6a1pre and 3.5 RC2 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1) Gecko/20090616 Firefox/3.5 Nice find Myk

Not gonna block final on this, as it's only through the Open Web Location dialog which isn't widely used. Should try to get it on the stability/security release cycle.

Taking...

We need to persist the pref to an in-memory variable for use inside the private browsing mode, just like the download last dir location, so that the usability of this dialog inside the private browsing mode does not regress.

Note: bug 287329 talks about making the dialog behave more like the location bar. Perhaps it would be easier to update the dialog to behave more like the bar than to maintain the existing functionality.

(In reply to comment #6) > Note: bug 287329 talks about making the dialog behave more like the location > bar. Perhaps it would be easier to update the dialog to behave more like the > bar than to maintain the existing functionality. That is out of scope for this bug. But even if the open location dialog gets modified to use something like the URL bar, we'd still may want to auto-fill it with the same value, so that might not solve the issue completely.

Patch + automated unit tests.

Comment on attachment 388058 [details] [diff] [review] Patch (v1) I'm not sure I get the design here. Why have the external JSM? That said, if you're going to have that, why not always get/set via gOpenLocationLastURL and have the private browsing checks all in the getter and setter?

(In reply to comment #9) > (From update of attachment 388058 [details] [diff] [review]) > I'm not sure I get the design here. Why have the external JSM? This was done to make the code more re-usable for extensions, and also throughout the codebase if we need to do this from another location some day. > That said, if > you're going to have that, why not always get/set via gOpenLocationLastURL and > have the private browsing checks all in the getter and setter? The design of gOpenLocationLastURL follows that of gDownloadLastDir <http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/src/DownloadLastDir.jsm#57>. This object has a single responsibility: to make sure that the contained path is not persistent after a private browsing mode switch. We can modify gOpenLocationLastURL to handle it all in its getters/setters, if you think that's more appropriate, but I personally wouldn't touch that for the sake of compatibility with gDownloadLastDir at least. That said, I'm willing to change the design if you still think it's necessary.

So if the idea is to be extensible/reusable, we shouldn't make the consumers need to be PB-aware, we should make the object smart and consumers simple. If you put the intelligence in the object, consumers just get/set a value and the Right Thing happens. That, to me, is an ideal case for any reuse... (I'm not sure why you used "compatibility" as an example. I guess you mean similarity of pattern, minimizing confusion? I think gDownloadLastDir was the wrong impl, and I think we should change it too.)

gDownloadLastDir is used in two separate places that both read the same pref value. They need to refer to the same in-memory value while in PB mode. Since that restriction doesn't apply in this case, I think we should just go with the simpler approach.

To be clear, by "simpler approach", I meant just putting the logic in openLocation.js and avoiding the module. But on second thought, I guess there can be two instances of that code running during a PB session, and if we care to maintain the current behavior in that case we'll need something like the JS module. It's not clear to me that we do, though...

(In reply to comment #13) > To be clear, by "simpler approach", I meant just putting the logic in > openLocation.js and avoiding the module. But on second thought, I guess there > can be two instances of that code running during a PB session, and if we care > to maintain the current behavior in that case we'll need something like the JS > module. It's not clear to me that we do, though... One simple example of this is when the user has two windows open. If we don't use a JSM, the last open dir in each window will be different, which is different to the behavior observed inside the private browsing mode. (In reply to comment #11) > So if the idea is to be extensible/reusable, we shouldn't make the consumers > need to be PB-aware, we should make the object smart and consumers simple. If > you put the intelligence in the object, consumers just get/set a value and the > Right Thing happens. That, to me, is an ideal case for any reuse... Hmm, I think you're right. Let's reach a final decision on whether or not to use a JSM and I'll update the patch accordingly then. > (I'm not sure why you used "compatibility" as an example. I guess you mean > similarity of pattern, minimizing confusion? I think gDownloadLastDir was the > wrong impl, and I think we should change it too.) Yes, by compatibility I meant similarity. I filed bug 508526 to make gDownloadLastDir smarter.

Let's do it in a JSM, so that PB works sanely across windows. (Sorry, meant to comment on this earlier.)

Use a JSM with a smart object.

Comment on attachment 394741 [details] [diff] [review] Patch (v2) Looks good, sorry for the delay.

Verified fixed on trunk with builds on Windows and OS X like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090831 Minefield/3.7a1pre ID:20090831030956

Looking at this, it's unclear to me what requires documentation here. This seems to be a deeply internal change. Can anyone suggest what needs documenting?

Verified fixed on 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a2pre) Gecko/20090901 Namoroka/3.6a2pre (.NET CLR 3.5.30729) ID:20090901050855

Comment on attachment 394741 [details] [diff] [review] Patch (v2) Approved for 1.9.1.4, a=dveditz for release-drivers

(In reply to comment #21) > Looking at this, it's unclear to me what requires documentation here. This > seems to be a deeply internal change. Can anyone suggest what needs > documenting? I added the dev-doc-needed keyword on this bug in order to document the gOpenLocationLastURL object.

This is now documented here: https://developer.mozilla.org/en/JavaScript_code_modules/openLocationLastURL.jsm

random orange test failure: Error 0 (Unknown error: 0).TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_openlocation.js | Exception thrown - [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch2.clearUserPref]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: chrome://mochikit/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_openlocation.js :: test :: line 91" data: no] TEST-UNEXPECTED-FAIL | /builds/moz2_slave/mozilla-1.9.1-macosx-unittest-everythingelse/build/xpcshell/tests/test_privatebrowsing/unit/test_openLocationLastURL.js | test failed (with xpcshell return code: 0), see following log: TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.clearUserPref]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: file:///builds/moz2_slave/mozilla-1.9.1-macosx-unittest-everythingelse/build/Shiretoko.app/Contents/MacOS/modules/openLocationLastURL.jsm :: anonymous :: line 91" data: no] Unknown Error: command finished with exit code: 1

the above random orange is happening on linux and mac

Could this be caused because clearUserPref throws an error if the value of the pref hasn't been modified? It looks like that. See bug 487059.

(In reply to comment #29) > Could this be caused because clearUserPref throws an error if the value of the > pref hasn't been modified? It looks like that. See bug 487059. Yes, I landed a fix for the failing tests as <http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e62d7ebe4a8e>.

Tree is green now. I can also verify the fix on 1.9.1 with builds on OSX and Windows like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.4pre) Gecko/20090907 Shiretoko/3.5.4pre ID:20090907040203