Closed Bug 385092 Opened 17 years ago Closed 17 years ago

Code evaluation not working

Categories

(Toolkit Graveyard :: Error Console, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9alpha7

People

(Reporter: robert.strong.bugs, Assigned: zeniko)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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
Hmm...  Could be, yes.  I should look into it, since the console _seems_ to be doing reasonable things....
Blocks: 372666
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.
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
(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.
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).
(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)
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"?
(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 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]
mozilla/toolkit/components/console/content/console.js 	1.10
Attachment #272462 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: [needs review comments addressed and checkin]
Blocks: tomtom
Product: Firefox → Toolkit
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: