Closed Bug 653108 Opened 9 years ago Closed 9 years ago

Scratchpad is tied to the tab it was first run in

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 6

People

(Reporter: AlexLakatos, Assigned: rcampbell)

References

Details

(Whiteboard: [scratchpad][fixed-in-devtools][merged-to-mozilla-central])

Attachments

(1 file, 6 obsolete files)

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.
Flags: in-testsuite?
Whiteboard: [workspaces]
Duplicate of this bug: 656421
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.
Whiteboard: [workspaces] → [scratchpad]
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.
OS: Windows XP → All
Hardware: x86 → All
Summary: workspace is tied to the tab it was first run in → Scratchpad is tied to the tab it was first run in
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Attached patch context hardening (obsolete) — Splinter Review
Made changes to getContentSandbox to cache previousBrowser and previousLocation. Now context is reset whenever tab is changed or location is changed. Includes a test.
Attachment #533700 - Flags: review?(mihai.sucan)
Attached patch context hardening (obsolete) — Splinter Review
now with test!
Attachment #533700 - Attachment is obsolete: true
Attachment #533700 - Flags: review?(mihai.sucan)
Attachment #533702 - Flags: review?(mihai.sucan)
really really added the test this time, honest
Attachment #533702 - Attachment is obsolete: true
Attachment #533702 - Flags: review?(mihai.sucan)
Attachment #533703 - Flags: review?(mihai.sucan)
Depends on: 657131
Attached patch context hardening rebased (obsolete) — Splinter Review
rebased after previous dependent bugs changed
Attachment #533703 - Attachment is obsolete: true
Attachment #533703 - Flags: review?(mihai.sucan)
Attachment #533724 - Flags: review?(mihai.sucan)
Attached patch context hardening (obsolete) — Splinter Review
fixed a merge error.
Attachment #533728 - Flags: review?(mihai.sucan)
Attached patch context hardening (obsolete) — Splinter Review
found a bug with the reset behavior. Fixed.
Attachment #533724 - Attachment is obsolete: true
Attachment #533728 - Attachment is obsolete: true
Attachment #533724 - Flags: review?(mihai.sucan)
Attachment #533728 - Flags: review?(mihai.sucan)
Attachment #533733 - Flags: review?(mihai.sucan)
Blocks: 658344
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.
Attachment #533733 - Flags: review?(mihai.sucan) → review+
(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é.
Attachment #533733 - Attachment is obsolete: true
Attachment #534068 - Flags: review?(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!
Attachment #534068 - Flags: review?(sdwilsh) → review+
Whiteboard: [scratchpad] → [scratchpad][fixed-in-devtools]
Comment on attachment 534068 [details] [diff] [review]
[checked-in] [in-devtools] context hardening

http://hg.mozilla.org/projects/devtools/rev/ca4df27501e4
Attachment #534068 - Attachment description: context hardening → [in-devtools] context hardening
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [scratchpad][fixed-in-devtools] → [scratchpad][fixed-in-devtools][merged-to-mozilla-central]
Target Milestone: --- → Firefox 6
Comment on attachment 534068 [details] [diff] [review]
[checked-in] [in-devtools] context hardening

http://hg.mozilla.org/mozilla-central/rev/ca4df27501e4
Attachment #534068 - Attachment description: [in-devtools] context hardening → [checked-in] [in-devtools] context hardening
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.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.