A Workspace is tied to the tab it first run in. If a second tab is opened and focused and code is executed, it will execute in the tab it was first executed. STR: 1.Open Workspace 2.Execute foo='bar'; foo+=foo; document.getElementsByTagName('body')[0].innerHTML=foo; 3.Open new tab and focus it 4.Execute the same code Expected Results: 4.The code is ran against the focused tab Actual Results: 4.The code is ran against the tab in Step 2 This happens if the tabs are both about:blank or two different tabs like cnn.com and bbc.com

This must be caused by the sandbox caching. We should clear the sandbox cache on tab switch.

agreed, though I thought we were checking this.browser == this.previousBrowser on evaluation. Might be some busted logic there.

a bit of looking at the code and the check we're doing is browserWindow == previousBrowserWindow (or similar names). We're not actually checking the tab. This is a pretty easy fix if someone wants to take a crack at it.

Another thing we should do is reset context if a user navigates away from the originating page. We don't want to cache variables between sites. We should also display the location of the currently active site in the status bar of the scratchpad, i.e., Content: http://wherever.com. Can probably do that in a follow-up.

Made changes to getContentSandbox to cache previousBrowser and previousLocation. Now context is reset whenever tab is changed or location is changed. Includes a test.

now with test!

really really added the test this time, honest

rebased after previous dependent bugs changed

fixed a merge error.

found a bug with the reset behavior. Fixed.

Comment on attachment 533733 [details] [diff] [review] context hardening Review of attachment 533733 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks good, but please address the comments below. r+ with that! ::: browser/base/content/scratchpad.js @@ +156,1 @@ > } Hm, this is fishy. I would expect, like in the comment below, that the status bar always shows the URL in which the script will execute, for the content context. However, for that, you'd need a focus event listener for the Scratchpad window such that when the user returns to the window, you can check the most recently active chrome window and determine the selected tab from there. Thoughts? @@ +617,1 @@ > }, I would expect the status bar to always show me the URL in which content context the script will execute. I would also expect that this method doesn't need me to give it the menuitem DOM node. ::: browser/base/content/test/browser_scratchpad_tab_switch.js @@ +58,5 @@ > + is(statusbar.getAttribute("label"), contentMenu.getAttribute("label"), > + "statusbar label is correct"); > + > + ok(sp.textbox, "textbox exists"); > + sp.textbox.value = "window.foobarBug636725 = 'aloha';"; foobarBug653108 or something else. ;) @@ +89,5 @@ > + sp.textbox.value = "window.foobarBug636725 = 'ahoyhoy';"; > + sp.run(); > + > + is(content.wrappedJSObject.foobarBug636725, "ahoyhoy", > + "content.foobarBug636725 has been set 2"); // fails Why // fails ? @@ +100,5 @@ > + gBrowser.selectedBrowser.removeEventListener("load", runTests3, true); > + // Check that the sandbox is not cached. > + > + sp.textbox.value = "typeof foobarBug636725;"; > + is(sp.run()[1], "undefined", "global variable does not exist"); // fails Why // fails? @@ +106,5 @@ > + gScratchpadWindow.close(); > + gScratchpadWindow = null; > + gBrowser.removeCurrentTab(); > + gBrowser.removeCurrentTab(); > + finish(); You do not clear the tab1, tab2 and sp variables. This will certainly cause memory leaks.

(In reply to comment #12) > Comment on attachment 533733 [details] [diff] [review] [review] > context hardening > > Review of attachment 533733 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > The patch looks good, but please address the comments below. r+ with that! > > ::: browser/base/content/scratchpad.js > @@ +156,1 @@ > > } > > Hm, this is fishy. I would expect, like in the comment below, that the > status bar always shows the URL in which the script will execute, for the > content context. > > However, for that, you'd need a focus event listener for the Scratchpad > window such that when the user returns to the window, you can check the most > recently active chrome window and determine the selected tab from there. > > Thoughts? I think the focus listener adds needless complexity here. When there is no URL, it means you have a "clean" environment. You're reset and the Scratchpad is ready to run against whichever tab you have selected. Some documentation and explanation in the forthcoming documentary video should help. > @@ +617,1 @@ > > }, > > I would expect the status bar to always show me the URL in which content > context the script will execute. I would also expect that this method > doesn't need me to give it the menuitem DOM node. That was a tad hacky, perhaps. I'll see what I can do. > ::: browser/base/content/test/browser_scratchpad_tab_switch.js > @@ +58,5 @@ > > + is(statusbar.getAttribute("label"), contentMenu.getAttribute("label"), > > + "statusbar label is correct"); > > + > > + ok(sp.textbox, "textbox exists"); > > + sp.textbox.value = "window.foobarBug636725 = 'aloha';"; > > foobarBug653108 or something else. ;) oh, Ok. ;) > @@ +89,5 @@ > > + sp.textbox.value = "window.foobarBug636725 = 'ahoyhoy';"; > > + sp.run(); > > + > > + is(content.wrappedJSObject.foobarBug636725, "ahoyhoy", > > + "content.foobarBug636725 has been set 2"); // fails > > Why // fails ? > > @@ +100,5 @@ > > + gBrowser.selectedBrowser.removeEventListener("load", runTests3, true); > > + // Check that the sandbox is not cached. > > + > > + sp.textbox.value = "typeof foobarBug636725;"; > > + is(sp.run()[1], "undefined", "global variable does not exist"); // fails > > Why // fails? Whups. These were leftover from when I initially wrote the test. I wrote the failures first then fixed the code. Didn't update comments afterwards. failure-driven-development? > @@ +106,5 @@ > > + gScratchpadWindow.close(); > > + gScratchpadWindow = null; > > + gBrowser.removeCurrentTab(); > > + gBrowser.removeCurrentTab(); > > + finish(); > > You do not clear the tab1, tab2 and sp variables. This will certainly cause > memory leaks. oh, good call. Thanks!

(In reply to comment #13) > (In reply to comment #12) > > Hm, this is fishy. I would expect, like in the comment below, that the > > status bar always shows the URL in which the script will execute, for the > > content context. > > > > However, for that, you'd need a focus event listener for the Scratchpad > > window such that when the user returns to the window, you can check the most > > recently active chrome window and determine the selected tab from there. > > > > Thoughts? > > I think the focus listener adds needless complexity here. When there is no > URL, it means you have a "clean" environment. You're reset and the > Scratchpad is ready to run against whichever tab you have selected. so, we riffed on this a bit in IRC and I think for now, the best approach is going to be removing the URL from the Content status line to minimize the changes here. Updating...

updated per review comments. All tests passé.

Comment on attachment 534068 [details] [diff] [review] [checked-in] [in-devtools] context hardening Review of attachment 534068 [details] [diff] [review]: ----------------------------------------------------------------- r=sdwilsh ::: browser/locales/en-US/chrome/browser/scratchpad.properties @@ +27,5 @@ > > # LOCALIZATION NOTE (saveFile.failed): This is the message displayed when file > # save fails. > saveFile.failed=The file save operation failed. > + extraneous change!

Comment on attachment 534068 [details] [diff] [review] [checked-in] [in-devtools] context hardening http://hg.mozilla.org/projects/devtools/rev/ca4df27501e4

Comment on attachment 534068 [details] [diff] [review] [checked-in] [in-devtools] context hardening http://hg.mozilla.org/mozilla-central/rev/ca4df27501e4

Verified fixed on: Windows 7: Mozilla/5.0 (Windows NT 6.1; rv:6.0a2) Gecko/20110525 Firefox/6.0a2 Window XP: Mozilla/5.0 (Windows NT 5.1; rv:6.0a2) Gecko/20110525 Firefox/6.0a2 Mac OS 10.6 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a2) Gecko/20110525 Firefox/6.0a2 Linux i686: Mozilla/5.0 (X11; Linux i686; rv:6.0a2) Gecko/20110525 Firefox/6.0a2 *Note: The issue is not longer present (used the steps in the description). Marking this as Verified.