Last Comment Bug 385092 - Code evaluation not working
: Code evaluation not working
Status: RESOLVED FIXED
: regression
Product: Toolkit Graveyard
Classification: Graveyard
Component: Error Console (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla1.9alpha7
Assigned To: Simon Bünzli
:
:
Mentors:
: 386501 (view as bug list)
Depends on:
Blocks: 372666 380422 tomtom
  Show dependency treegraph
 
Reported: 2007-06-19 15:15 PDT by Robert Strong [:rstrong] (use needinfo to contact me)
Modified: 2016-06-29 11:02 PDT (History)
11 users (show)
mconnor: blocking1.9+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
run the JS code once about:config had been loaded (1.30 KB, patch)
2007-07-03 01:24 PDT, Simon Bünzli
no flags Details | Diff | Splinter Review
evaluate from displayResult, once about:blank has been loaded (3.73 KB, patch)
2007-07-16 01:47 PDT, Simon Bünzli
gavin.sharp: review+
Details | Diff | Splinter Review
updated to comments (3.73 KB, patch)
2007-07-24 07:57 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review

Description User image Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-19 15:15:55 PDT
Checked with the latest nightly on Win32 (my system), Linux (bkap's system), and Mac OS X (dcamp's system).

To Reproduce:
1. Open Error Console
2. In the Code field enter alert('test');

Actual Results
No alert is displayed

Expected Results
Alert is displayed
Comment 1 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2007-06-19 17:19:29 PDT
The regression range I get is:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&branchtype=match&date=explicit&mindate=2007-06-10+02%3A00&maxdate=2007-06-11+05%3A00

Caused by bug 372666, perhaps?
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2007-06-19 17:33:37 PDT
Hmm...  Could be, yes.  I should look into it, since the console _seems_ to be doing reasonable things....
Comment 3 User image neil@parkwaycc.co.uk 2007-06-20 04:07:12 PDT
If you look in DOM Inspector the URL for the evaluator document is always about:blank but in earlier builds it is the javascript: that you evaluated.
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2007-06-20 04:16:21 PDT
Right.  Need to figure out why it ends up with the mFiredUnloadEvent state...

I'm probably not going to be able to do this for a few weeks.
Comment 5 User image Simon Bünzli 2007-07-01 09:04:30 PDT
*** Bug 386501 has been marked as a duplicate of this bug. ***
Comment 6 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2007-07-01 09:08:11 PDT
*** Bug 386501 has been marked as a duplicate of this bug. ***
Comment 7 User image Wladimir Palant 2007-07-01 09:10:26 PDT
I attached a patch in bug 386501 but I guess adding a timeout is not what we want to do here?
Comment 8 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2007-07-01 09:35:20 PDT
We need to figure out why something that previously worked broke due to the patch for bug 372666. Sounds like you're right that it's related to bug 380422.
Comment 9 User image Boris Zbarsky [:bz] (still a bit busy) 2007-07-01 09:50:03 PDT
Oh, bug 372666.  I thought this had regressed from bug 371360.

Nothing to figure out, then.  The console code as written starts a load, then starts a javascript: load.  With async javascript: loads, the javascript actually runs after the first load has completed, which means that it doesn't run at all due to the fix for bug 372666.

This could turn out to be a web compat issue, but I kind of doubt it.  So I think the right fix is to not do the javascript: load here until the about:blank load actually finishes.

And yes, the fix for bug 380422 was basically wrong, imo.
Comment 10 User image Simon Bünzli 2007-07-02 01:41:02 PDT
(In reply to comment #9)
> And yes, the fix for bug 380422 was basically wrong, imo.

What would have been a better fix then?

> var evaluator = document.getElementById("Evaluator");
> evaluator.setAttribute("src", "about:blank"); // reset the iframe
> evaluator.setAttribute("src", "javascript: " + code);

has become broken in the same way as the current code, even though bug 238898 comment #9 seems to tell otherwise.

> var evaluator = document.getElementById("Evaluator").contentWindow;
> evaluator.addEventListener("load", function () {
>   evaluator.removeEventListener("load", arguments.callee, true);
>   evaluator.location = "javascript: " + code;
> }, true);
> evaluator.location = "about:blank"; // reset the iframe

doesn't work either, since the load event seems not to be fired at all when evaluator.location == "about:blank" (nor is it fired on evaluator.location.reload() ).

> var evaluator = document.getElementById("Evaluator").contentWindow;
> evaluator.location = "about:blank"; // reset the iframe
> setTimeout(function() {
>   evaluator.location = "javascript: " + code;
> }, 0);

works mostly, although it looks like a hack and seems to intermittently drop entries as well when [Enter] is kept pressed down.

So, either (1) this is an issue introduced with bug 372666 and should be fixed there, (2) there's a non-obvious solution we've been missing, (3) we want to add a new feature for more clearly reset a window's JS context, or (4) we use Wladimir's patch (attachment #270496 [details] [diff] [review]) after all...
Comment 11 User image Jeff Walden [:Waldo] (remove +bmo to email) 2007-07-02 02:12:27 PDT
(In reply to comment #10)
> What would have been a better fix then?

Probably nsIWebNavigation for the window, which you could use to load the JS URL after getting a document-loaded notification or similar.  Of course, probably the easiest way would be to just make a new iframe each time and blow away the old one, and that's what I'd do here.
Comment 12 User image Boris Zbarsky [:bz] (still a bit busy) 2007-07-02 13:41:51 PDT
> What would have been a better fix then?

See comment 9 paragraph 3.

Setting an onload listener should work if you set it on the iframe itself, not on the window inside it (since the window gets blown away when a load happens).  

To clarify, bug 372666 removed a race condition where one site could execute JS against another site by starting a javascript: load in parallel with some other load.  Unfortunately, this code was relying on exactly that race condition to run JS against the new site (in this case, the result of loading about:blank).
Comment 13 User image Simon Bünzli 2007-07-03 01:24:58 PDT
Created attachment 270705 [details] [diff] [review]
run the JS code once about:config had been loaded

(In reply to comment #12)
> Setting an onload listener should work if you set it on the iframe itself
Hmm, thought I had done that. This indeed works.
Comment 14 User image neil@parkwaycc.co.uk 2007-07-03 05:07:14 PDT
Although that is confusing as we already have one load event listener on the frame (so that we know when the JavaScript has been evaluated).
Comment 15 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2007-07-15 15:46:27 PDT
(In reply to comment #14)
> Although that is confusing as we already have one load event listener on the
> frame (so that we know when the JavaScript has been evaluated).

Indeed, that is a bit confusing. Can we get rid of the other listener, and have the one you're adding in this patch call displayResult() if iframe.contentWindow.location != "about:blank"?
Comment 16 User image Simon Bünzli 2007-07-16 01:47:54 PDT
Created attachment 272462 [details] [diff] [review]
evaluate from displayResult, once about:blank has been loaded

(In reply to comment #15)
> have the one you're adding in this patch call displayResult()

That won't work, since the load listener seems not to be guaranteed to be called for defective javascript: URLs. We can however do it the other way around: load the javascript: URL from displayResult once about:blank has been loaded.
Comment 17 User image neil@parkwaycc.co.uk 2007-07-23 03:30:02 PDT
Comment on attachment 272462 [details] [diff] [review]
evaluate from displayResult, once about:blank has been loaded

>+  if (gEvaluator.contentWindow.location.href == "about:blank") {
Wouldn't it be easier to test for gCodeToEvaluate?
Comment 18 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2007-07-23 15:02:41 PDT
Comment on attachment 272462 [details] [diff] [review]
evaluate from displayResult, once about:blank has been loaded

Test for gCodeToEvaluate, as Neil mentions, and r=me. Might also want to change the name of displayResult to something like "loadOrDisplayResult"?
Comment 19 User image Mike Connor [:mconnor] 2007-07-23 23:32:46 PDT
Gavin, can you update the patch for your review comments and land this before freeze?  If not, please punt to M8 so Simon can do so when he's back.
Comment 20 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2007-07-24 07:57:33 PDT
Created attachment 273589 [details] [diff] [review]
updated to comments

mozilla/toolkit/components/console/content/console.js 	1.10

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