385092 - Code evaluation not working

Status: RESOLVED FIXED

(Toolkit Graveyard :: Error Console, defect)

Description

[Robert Strong (they/them - no direct email)]

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

[:Gavin Sharp [email: [e-mail]]]

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

[Boris Zbarsky [:bzbarsky]]

Hmm...  Could be, yes.  I should look into it, since the console _seems_ to be doing reasonable things....

Comment 3

[[e-mail]]

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

[Boris Zbarsky [:bzbarsky]]

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 7

[Wladimir Palant]

I attached a patch in bug 386501 but I guess adding a timeout is not what we want to do here?

Comment 8

[:Gavin Sharp [email: [e-mail]]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Simon Bünzli]

(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

[Jeff Walden [:Waldo]]

(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

[Boris Zbarsky [:bzbarsky]]

> 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

[Simon Bünzli]

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

[[e-mail]]

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

[:Gavin Sharp [email: [e-mail]]]

(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

[Simon Bünzli]

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

[[e-mail]]

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

[:Gavin Sharp [email: [e-mail]]]

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

[Mike Connor [:mconnor]]

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

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated to comments

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