653108 - Scratchpad is tied to the tab it was first run in

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Alex Lakatos[:AlexLakatos]]

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]]

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

Comment 2

[Rob Campbell [:rc] (:robcee)]

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

Comment 4

[Rob Campbell [:rc] (:robcee)]

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)]

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)]

Attachment: 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)]

Attachment: context hardening

now with test!

Comment 8

[Rob Campbell [:rc] (:robcee)]

Attachment: context hardening with test really this time

really really added the test this time, honest

Comment 9

[Rob Campbell [:rc] (:robcee)]

Attachment: context hardening rebased

rebased after previous dependent bugs changed

Comment 10

[Rob Campbell [:rc] (:robcee)]

Attachment: context hardening

fixed a merge error.

Comment 11

[Rob Campbell [:rc] (:robcee)]

Attachment: context hardening

found a bug with the reset behavior. Fixed.

Comment 12

[Mihai Sucan [:msucan]]

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)]

(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)]

(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)]

Attachment: [checked-in] [in-devtools] context hardening

updated per review comments. All tests passé.

Comment 16

[Shawn Wilsher :sdwilsh]

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)]

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)]

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]]

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.