Last Comment Bug 653108 - Scratchpad is tied to the tab it was first run in
: Scratchpad is tied to the tab it was first run in
Status: VERIFIED FIXED
[scratchpad][fixed-in-devtools][merge...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 6
Assigned To: Rob Campbell [:rc] (:robcee)
:
Mentors:
: 656421 (view as bug list)
Depends on: 657131
Blocks: 658344
  Show dependency treegraph
 
Reported: 2011-04-27 06:43 PDT by Alex Lakatos[:AlexLakatos]
Modified: 2011-05-26 07:15 PDT (History)
6 users (show)
sdwilsh: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
context hardening (5.86 KB, patch)
2011-05-19 10:35 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Review
context hardening (5.86 KB, patch)
2011-05-19 10:37 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Review
context hardening with test really this time (9.66 KB, patch)
2011-05-19 10:39 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Review
context hardening rebased (9.72 KB, patch)
2011-05-19 11:29 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Review
context hardening (9.67 KB, patch)
2011-05-19 11:35 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Review
context hardening (9.87 KB, patch)
2011-05-19 11:46 PDT, Rob Campbell [:rc] (:robcee)
mihai.sucan: review+
Details | Diff | Review
[checked-in] [in-devtools] context hardening (8.82 KB, patch)
2011-05-20 12:36 PDT, Rob Campbell [:rc] (:robcee)
sdwilsh: review+
Details | Diff | Review

Description Alex Lakatos[:AlexLakatos] 2011-04-27 06:43:50 PDT
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
Comment 1 Mihai Sucan [:msucan] 2011-04-27 07:48:18 PDT
This must be caused by the sandbox caching. We should clear the sandbox cache on tab switch.
Comment 2 Rob Campbell [:rc] (:robcee) 2011-04-27 08:25:32 PDT
agreed, though I thought we were checking this.browser == this.previousBrowser on evaluation. Might be some busted logic there.
Comment 3 Rob Campbell [:rc] (:robcee) 2011-05-11 18:41:10 PDT
*** Bug 656421 has been marked as a duplicate of this bug. ***
Comment 4 Rob Campbell [:rc] (:robcee) 2011-05-11 18:42:58 PDT
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.
Comment 5 Rob Campbell [:rc] (:robcee) 2011-05-14 07:54:07 PDT
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.
Comment 6 Rob Campbell [:rc] (:robcee) 2011-05-19 10:35:40 PDT
Created attachment 533700 [details] [diff] [review]
context hardening

Made changes to getContentSandbox to cache previousBrowser and previousLocation. Now context is reset whenever tab is changed or location is changed. Includes a test.
Comment 7 Rob Campbell [:rc] (:robcee) 2011-05-19 10:37:54 PDT
Created attachment 533702 [details] [diff] [review]
context hardening

now with test!
Comment 8 Rob Campbell [:rc] (:robcee) 2011-05-19 10:39:18 PDT
Created attachment 533703 [details] [diff] [review]
context hardening with test really this time

really really added the test this time, honest
Comment 9 Rob Campbell [:rc] (:robcee) 2011-05-19 11:29:04 PDT
Created attachment 533724 [details] [diff] [review]
context hardening rebased

rebased after previous dependent bugs changed
Comment 10 Rob Campbell [:rc] (:robcee) 2011-05-19 11:35:17 PDT
Created attachment 533728 [details] [diff] [review]
context hardening

fixed a merge error.
Comment 11 Rob Campbell [:rc] (:robcee) 2011-05-19 11:46:01 PDT
Created attachment 533733 [details] [diff] [review]
context hardening

found a bug with the reset behavior. Fixed.
Comment 12 Mihai Sucan [:msucan] 2011-05-20 10:01:03 PDT
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.
Comment 13 Rob Campbell [:rc] (:robcee) 2011-05-20 10:07:42 PDT
(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!
Comment 14 Rob Campbell [:rc] (:robcee) 2011-05-20 10:20:24 PDT
(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...
Comment 15 Rob Campbell [:rc] (:robcee) 2011-05-20 12:36:49 PDT
Created attachment 534068 [details] [diff] [review]
[checked-in] [in-devtools] context hardening

updated per review comments. All tests passé.
Comment 16 Shawn Wilsher :sdwilsh 2011-05-20 13:12:58 PDT
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 17 Rob Campbell [:rc] (:robcee) 2011-05-21 05:40:25 PDT
Comment on attachment 534068 [details] [diff] [review]
[checked-in] [in-devtools] context hardening

http://hg.mozilla.org/projects/devtools/rev/ca4df27501e4
Comment 18 Dave Camp (:dcamp) 2011-05-21 21:30:13 PDT
Comment on attachment 534068 [details] [diff] [review]
[checked-in] [in-devtools] context hardening

http://hg.mozilla.org/mozilla-central/rev/ca4df27501e4
Comment 19 AndreiD[QA] 2011-05-26 07:15:13 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.