Code evaluation not working

RESOLVED FIXED in mozilla1.9alpha7

Status

Toolkit Graveyard
Error Console
RESOLVED FIXED
10 years ago
11 months ago

People

(Reporter: rstrong, Assigned: Simon Bünzli)

Tracking

({regression})

Trunk
mozilla1.9alpha7
regression
Dependency tree / graph
Bug Flags:
blocking1.9 +

Details

Attachments

(1 attachment, 2 obsolete attachments)

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
Flags: blocking-firefox3?
OS: Windows Vista → All
Hardware: PC → All
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?
Hmm...  Could be, yes.  I should look into it, since the console _seems_ to be doing reasonable things....
Blocks: 372666

Comment 3

10 years ago
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.
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.
(Assignee)

Updated

10 years ago
Duplicate of this bug: 386501
Duplicate of this bug: 386501

Comment 7

10 years ago
I attached a patch in bug 386501 but I guess adding a timeout is not what we want to do here?
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.
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.
Blocks: 380422
(Assignee)

Comment 10

10 years ago
(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...
(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.

Updated

10 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 beta1
> 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).
(Assignee)

Comment 13

10 years ago
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.
Assignee: nobody → zeniko
Status: NEW → ASSIGNED
Attachment #270705 - Flags: review?(gavin.sharp)

Comment 14

10 years ago
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).
(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"?
(Assignee)

Comment 16

10 years ago
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.
Attachment #270705 - Attachment is obsolete: true
Attachment #272462 - Flags: review?(gavin.sharp)
Attachment #270705 - Flags: review?(gavin.sharp)

Comment 17

10 years ago
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 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"?
Attachment #272462 - Flags: review?(gavin.sharp) → review+
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.
Whiteboard: [needs review comments addressed and checkin]
Created attachment 273589 [details] [diff] [review]
updated to comments

mozilla/toolkit/components/console/content/console.js 	1.10
Attachment #272462 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs review comments addressed and checkin]

Updated

10 years ago
Blocks: 393966
Product: Firefox → Toolkit
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.