bug in Safari? eval scope / context

[Liyang HU]

Sorry. Me again.

Found a slight issue with TiddlyWiki's (loadPlugins()'s) use of
eval(). There's a line along the lines of:

window.eval(tiddler.text);

which ought to evaluate the contents of tiddler.text under the
restricted context of window. That doesn't quite work as expected in
Safari (and by extension, I'm guessing all the KDE stuff): Safari
ignores the context and just evaluates the code in the context of
loadPlugins(), as if it just said eval(tiddler.text);. This caused
much hilarity[0].

This is what Mozilla says about eval:
http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Global_Functions:eval

Apparently the new way to achieve the above is to pass the restricted
context as the second parameter to the global eval() function, a la:
eval(tiddler.text, window);

But I'm sure all the other implementations say something different.
Not exactly portable.

Here's my proposed fix:

(function() { eval(tiddler.text); })();

Create an anonymous function / scope to call eval(), and invoke it
immediately. (Then discard it.) You probably don't want one plugin to
be able to leave garbage lying about for a later plugin to pick up,
which evaluating it under window would do.

Incidentally, which version/implementation of ECMAScript is TiddlyWiki
targeting? (Everything?)

Cheers,
/Liyang

[0] loadPlugins() uses i as a counter. I like using the letter i as a
counter. Hilarity ensues.

[Liyang HU]

On Jan 4, 9:40 am, Liyang HU <liyang...@gmail.com> wrote:
> Apparently the new way to achieve the above is to pass the restricted
> context as the second parameter to the global eval() function, a la:
> eval(tiddler.text, window);

I should note that this does work in Safari. But (function() { return
eval(...); })(); would be better.


There were a few more changes before I could manage to get TiddlyWiki
to run as XHTML/JavaScript without errors / warnings. See below.

Cheers,
/Liyang -- I'll get the SVN repo next time.

--- index.20080104.0955560298.html 2008-01-04 09:55:56.000000000
+0000
+++ index.xhtml 2008-01-04 10:08:59.000000000 +0000
@@ -6822,6 +6820,8 @@
{
if(safeMode)
return;
+ if(!document.cookie)
+ return;
var cookies = document.cookie.split(";");
for(var c=0; c<cookies.length; c++) {
var p = cookies[c].indexOf("=");
@@ -9426,7 +9426,8 @@
function removeNode(e)
{
scrubNode(e);
- e.parentNode.removeChild(e);
+ if(e.parentNode)
+ e.parentNode.removeChild(e);
}

// Remove any event handlers or non-primitve custom attributes
@@ -9674,8 +9675,10 @@
</script>
<script type="text/javascript">
//<![CDATA[
-if(useJavaSaver)
+if(useJavaSaver && document.write)
+{
document.write("<applet style='position:absolute;left:-1px'
name='TiddlySaver' code='TiddlySaver.class' archive='TiddlySaver.jar'
width='1' height='1'></applet>");
+}
//]]>
</script>
<!--POST-SCRIPT-START-->

[Jeremy Ruston]

These are good suggestions. It would be terrific to create tickets for
them at http://trac.tiddlywiki.org/.

Changing the use of eval() is sound, although as a word of warning,
I've found in the past that this is an area that's extremely hard to
change without breaking existing plugins on some browser or another
(sometimes quite subtly). In the past, we did at one point try a
slightly different approach of something like:

window.eval("function() {" + tiddler.text+ "}();");

The idea was to better isolate plugins, and also to enable them to
return an exit code via the 'return' statement. As I recall it, we had
to pull the change quickly to accommodate one or the other of the
clunkier browsers (probably Safari).

This time around, if we're going to change this, I guess it just needs
to be as part of a reasonable beta programme.

Cheers

Jeremy

--
Jeremy Ruston
mailto:jer...@osmosoft.com
http://www.tiddlywiki.com

[FND]

> These are good suggestions. It would be terrific to create tickets for
> them at http://trac.tiddlywiki.org/.

I have taken the liberty of doing so:
http://trac.tiddlywiki.org/ticket/443
It's classified as a defect for now - not entirely sure whether that's
correct. Same with the priority and severity of this issue.

-- F.

[Eric Shulman]

> Changing the use of eval() is sound, although as a word of warning,
> I've found in the past that this is an area that's extremely hard to
> change without breaking existing plugins on some browser or another
> (sometimes quite subtly). In the past, we did at one point try a
> slightly different approach of something like:
>
> window.eval("function() {" + tiddler.text+ "}();");
>
> The idea was to better isolate plugins, and also to enable them to
> return an exit code via the 'return' statement. As I recall it, we had
> to pull the change quickly to accommodate one or the other of the
> clunkier browsers (probably Safari).

Actually, while the suggested change may seem innocuous, it will
create serious problems for existing plugins:

MANY bits of javascript code assume that something like this:
myStateFlag = true;
creates a variable in the GLOBAL scope (i.e., window.myStateFlag).
This is typically used to track plugin-specific stateful information
that needs to PERSIST between invocations of their plugin code.

This usage could be corrected by re-writing each plugin so that
references to global variables always use an explicit "window." prefix
to specify the scope. However, that would create a HUGE impact on
existing code, and would create a major schism between versions: any
plugins that rely on creating global values that are already installed
in existing documents would BREAK when migrating to the new core,
making upgrades a real headache for everyone, not just plugin
developers!

As the author (and maintainer) of nearly 160 plugins and scripts, the
impact on my code alone would be extremely distruptive and unless
there is a compelling reason to make this change, I strongly recommend
NOT doing it.

-e
Eric Shulman
TiddlyTools / ELS Design Studios

[Saq Imtiaz]

I'm with Eric on this one. Evaluating systemConfig code in the window context and not wrapping it in an anonymous function was a conscious decision due to the very reasons that Eric has mentioned.

 

Instead of using window.eval, couldn't we call eval using call or apply and thus pass it the correct scope, i.e. window?

 

As for the garbage collection after loadPlugins, that is a problem. It leaves some hanging variables as I believed I mentioned to Martin a few months back, but  I forgot to ticket it. One of the said variables is tiddler. The global variable tiddler has the value of the last systemConfig tiddler load by loadPlugins. However, we should be able to unset those variables without much hassle I would think.

 

Saq

--
Tiddly Learning ( http://lewcid.org ) : TiddlyWiki news, plugins, themes and educational usage

[Eric Shulman]

> As for the garbage collection after loadPlugins, that is a problem. It
> leaves some hanging variables as I believed I mentioned to Martin a few
> months back, but I forgot to ticket it. One of the said variables is
> tiddler. The global variable tiddler has the value of the last systemConfig
> tiddler load by loadPlugins. However, we should be able to unset those
> variables without much hassle I would think.

If 'tiddler' actually referred to the *current* tiddler object, this
variable could be VERY useful in evaluating "computed macro
parameters". For example:

<<newTiddler label:"New Notes" title:{{tiddler.title+"Notes"}}>>

could be used in a tiddler to add a "New Notes" command that would
create a separate tiddler, using the current tiddler's title as a
prefix.

Another global that would also be incredibly useful in computed params
is: "place" (i.e., the current rendering location for the macro). The
"place" variable IS provided to the macro's handler() function, but is
NOT similarly available within computed parameters...

UNTIL NOW THAT IS!

I've just added a new CoreTweak at TiddlyTools that extends the core's
invokeMacro() function to automatically set these two global variables
(tiddler and place), making them available for use in computed macro
parameters... for any macro.

Get the update here:
http://www.TiddlyTools.com/#CoreTweaks

I think this might be a good candidate for inclusion in the core...

[FND]

> If 'tiddler' actually referred to the *current* tiddler object, this
> variable could be VERY useful in evaluating "computed macro
> parameters".

> [...]

> Another global that would also be incredibly useful in computed params
> is: "place" (i.e., the current rendering location for the macro).

Indeed - "tiddler" and "place" would be immensely powerful tools to have
in that context!

> I've just added a new CoreTweak at TiddlyTools that extends the core's
> invokeMacro() function to automatically set these two global variables

> [...]

> I think this might be a good candidate for inclusion in the core...

I agree, and have thus created a ticket and patch for this issue:
http://trac.tiddlywiki.org/ticket/444
The patch is entirely based upon Eric's code, but I'm not sure whether
my implementation is correct - so a quick review would be welcome.

-- F.

[Eric Shulman]

> > I've just added a new CoreTweak at TiddlyTools that extends the core's
> > invokeMacro() function to automatically set these two global variables
> > [...]
> > I think this might be a good candidate for inclusion in the core...
> I agree, and have thus created a ticket and patch for this issue:
> http://trac.tiddlywiki.org/ticket/444
> The patch is entirely based upon Eric's code, but I'm not sure whether
> my implementation is correct - so a quick review would be welcome.

It looks OK... except you don't need to keep the 'hijack' code at the
end... that's was only required to implement the change as a
CoreTweak.

-e

[FND]

> you don't need to keep the 'hijack' code at the end

D'oh! That's a remnant from copying your code into my working copy.
I've fixed it now - thanks for the reminder!

-- F.

[Liyang HU]

Hi Eric et al.,

On Jan 5, 3:06 pm, Eric Shulman <elsdes...@gmail.com> wrote:
> MANY bits of javascript code assume that something like this:
> myStateFlag = true;
> creates a variable in the GLOBAL scope (i.e., window.myStateFlag).
> This is typically used to track plugin-specific stateful information
> that needs to PERSIST between invocations of their plugin code.

Don't think it's going to cause any issues on that front. I may have
misunderstood what you're trying to say, but using:

(function() { eval(tiddler.text); })();

creates a private / anonymous scope that only the plugin has access
to. (My previous post might not have been quite Technically Correct on
this point.) 'State' variables (or 'global', from the perspective of
the plugin) work just fine. Private plugin state stays private to the
plugin; I find this a desirable property.


As you said, it *does* break e.g. (what I can see here)
CryptoFunctionsPlugin trying to export functionality. I've fixed it in
my copy at http://liyang.hu/t/ (append index.html if your browser gets
confused).

However, plugins written with explicit references to "window." would
work perfectly fine, whichever way it gets eval()'d.


> [...] unless there is a compelling reason to make this change, I strongly recommend
> NOT doing it.

Is it better to have everything global? (And having to worry about
name clashes.) Or to explicitly export those parts that need to be
global? I'm sure this is a settled Software Engineering issue, so I
won't carry on here.


I'll grant you that it'd be disruptive to change core TiddlyWiki, and
that's probably a good enough reason to hold off making such a change
in the short term. Start with the plugins. :)


Cheers,
/Liyang - who believes the best way to fix software is to rewrite it
from scratch. :3

[Liyang HU]

Hi again, :)

On Jan 5, 4:22 pm, Eric Shulman <elsdes...@gmail.com> wrote:
> Another global that would also be incredibly useful in computed params
> is: "place" (i.e., the current rendering location for the macro). The
> "place" variable IS provided to the macro's handler() function, but is
> NOT similarly available within computed parameters...

I do worry about this excitement over global variables. I (at least)
feel that globals ought to hold information about things that are
applicable *globally*, and frankly 'tiddler' and 'place' aren't. (They
apply to the current tiddler only.)


It seems like what you're after here are named, implicit parameters.
(Which can be ignored by the macro unless needed.) I can think of two
ways of doing this without the use of globals:

1. Pass a dictionary as the first (or last?) argument, with fields
named 'tiddler' and 'place'. This is perhaps a somewhat disruptive and
inconvenient way to implement this idea.

2. What I'm suggesting instead - invoke the all-powerful eval():

function() { var tiddler = ..., place = ...; return
eval(macroText); } ();


Cheers,
/Liyang - who feels suitably eval^H^Hil now.

[Liyang HU]

Oh dear. I'm replying to myself now.

On Jan 6, 6:26 pm, Liyang HU <liyang...@gmail.com> wrote:
> 2. What I'm suggesting instead - invoke the all-powerful eval():

> function() { var tiddler = ..., place = ...; return
> eval(macroText); } ();

Hadn't dawned on me that macros aren't found in tiddlers, and eval
isn't applicable. (Or is it? I haven't checked the code. Apologies for
being sloppy. Just trying to throw some ideas around.)

Maybe this would work:

var that = new function() { this.tiddler = ...; this.place = ...;
return this; } ();
macroHandler.call(this, ...);

/Liyang :3
PS: this is the second attempt. In the previous post I omitted 'new'.

[Eric Shulman]

> I do worry about this excitement over global variables. I (at least)
> feel that globals ought to hold information about things that are
> applicable *globally*, and frankly 'tiddler' and 'place' aren't. (They
> apply to the current tiddler only.)

"tiddler" and "place" aren't "applied to the current tiddler"... they
DEFINE the current tiddler within the *global* state of the TiddlyWiki
system... IMO, the use of globals in this context seems absolutely
appropriate.

> It seems like what you're after here are named, implicit parameters.
> (Which can be ignored by the macro unless needed.) I can think of two
> ways of doing this without the use of globals:

It is NOT the macro that needs these global state values... it is the
core's macro engine that needs them to eval any 'computed' macro
parameters BEFORE passing the resulting values on to the macro
handler.

> 2. What I'm suggesting instead - invoke the all-powerful eval():
>
> function() { var tiddler = ..., place = ...; return
> eval(macroText); } ();

When you say, "I feel that globals ought...", this tells me that you
are engaging in philosophical discussion! I thought we were trying to
address *practical* concerns.

While it may be philosophically objectionable to you, it is *standard
practice* in javascript (not just in TiddlyWiki) for "window" to be
the default scope for non-local variables (i.e. not declared with
"var").

A great deal of existing code (both in TiddlyWiki and otherwise)
relies upon this aspect of javascript to create values that implicitly
persist in the global (window) space.

While the use of globals *could* be re-written in each and every
instance so that the references are explicit, your suggestion of
wrapping the eval() in an anonymous function to deliberately prevent
implicit reference to this global scope is not the right approach...
it would break TONS of existing code, and would not really improve
anything.

In addition, there are browser-related issues that would still require
the use of globals: in IE for example, functions that are dynamically-
defined via eval() and then invoked as event handlers directly from
DOM elements need to be explicitly globally-scoped or IE can throw an
error. Thus, it is simply NOT possible for plugins to avoid creating
ANY globals...

Lastly: if YOU are encountering problems because of a 'leftover'
global such as "i", then YOUR code is already using the implicit
global scope (probably without intending to). To address this in, you
should:

A) make sure to always initialize your variables before using them,
and
B) precede your variables with "var" so that they are locally-scoped,
and won't reference the globally-scoped value.

In summary: Making such a low-level, pervasive change in the core's
macro handling on the basis of a philosophical concern, rather than as
a solution to a real problem, seems foolhardly and unnecessary, and
IMO, would be a *very big mistake*.

[Liyang HU]

Hi,

On Jan 6, 7:39 pm, Eric Shulman <elsdes...@gmail.com> wrote:
> When you say, "I feel that globals ought...", this tells me that you
> are engaging in philosophical discussion! I thought we were trying to
> address *practical* concerns.

Practical? Me? I am studying for a doctorate of philosophy, after
all. :3

My personal philosophy is that writing code is an artform and what one
writes ought to be beautiful in the mathematical sense. I'd happily
spending a couple of days rewriting TiddlyWiki were I in a position to
do so, but fair enough: other people have better things to do. :)

> While it may be philosophically objectionable to you, it is *standard
> practice* in javascript (not just in TiddlyWiki) for "window" to be
> the default scope for non-local variables (i.e. not declared with
> "var").

Standard practice is what's discouraging me from going anywhere near
jobs in software engineering. (Though financial concerns may mean that
I will have to one day; I *have* served time in industry before.) I
think we may have to agree to disagree on this. ;)

I'm not suggesting this makes practical sense for TiddlyWiki now. Just
something to keep in mind for the future, be it TiddlyWiki or another
project.

> In addition, there are browser-related issues that would still require
> the use of globals: in IE for example, functions that are dynamically-
> defined via eval() and then invoked as event handlers directly from
> DOM elements need to be explicitly globally-scoped or IE can throw an
> error. Thus, it is simply NOT possible for plugins to avoid creating
> ANY globals...

Personal curiosity on this point - would this not work in IE?

eval(" ... window.randomEventHandler = function() { ... } ...");

(I don't have easy access to IE for testing right now.)

> A) make sure to always initialize your variables before using them,
> and
> B) precede your variables with "var" so that they are locally-scoped,
> and won't reference the globally-scoped value.

I always do.

In this case - I did - with for(var i = 0; i < ...; ...), but that was
still changing the value of i in loadPlugins()! (I wasn't quite so
clear on JavaScript's scoping rules; I am now and I guess it is
behaving as expected.) I've been bitten by uninitialised variable bugs
enough times to know that much. ;p

Were I to take this approach, I'd wrap the whole thing in an anonymous
function.


Happy Tiddling,
/Liyang

[Martin Budden]

I'm inclined to agree with Liyang - I think TiddlyWiki over-uses
global variables. But we are where we are and I don't favor taking
actions that break existing plugins. The unfortunate fact of the
matter is that there are some things we cannot really fix, because the
cure is worse than the disease.

Martin

[FND]

> I'm inclined to agree with Liyang [...]

> The unfortunate fact of the matter is that there are some things we
> cannot really fix, because the cure is worse than the disease.

FWIW, I agree.

Just out of curiosity:
This whole discussion, and the backwards compatibility issues in
particular, seem to revolve around the anonymous function.
What about the alternative solution though, using
eval(tiddler.text, context);
as suggested by Liyang, and elaborated on by Saq:

> Instead of using window.eval, couldn't we call eval using call or
> apply and thus pass it the correct scope, i.e. window?

Or is that basically the same?

-- F.

[Jeremy Ruston]

I think that there's still a problem left that we need to fix, which
is that local variables within the loadPlugins() function are visible
to the executed code within the plugin. Can we do fix it by wrapping
the window.eval() call up in a small function?

Cheers

Jeremy

--