171030 - (dompollution) Support global reference to a form via its name attribute [1.0 BRANCH ONLY]

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect, P1)

Description

[Doron Rosenberg (IBM)]

I was asked (not at gun point, mind you) to file this bug.

Tracks: http://bugscape.netscape.com/show_bug.cgi?id=20038

Comment 1

[Doron Rosenberg (IBM)]

lalalalalalalalala, mind the gap

Comment 2

[timeless]

amusing, quite amusing.

Comment 3

[Doron Rosenberg (IBM)]

Reopening

Comment 4

[Johnny Stenback (:jst)]

Attachment: Propsed fix (behind a pref, and works for https sites only, don't ask).

Comment 5

[John Bandhauer]

This code looks reasonable to me (ignoring the question of why it even exists :)

The only oddity that struck me was:

+  rv = nsDOMClassInfo::WrapNative(cx, ::JS_GetGlobalObject(cx), result,
+                                  NS_GET_IID(nsISupports), &val);

Isn't ::JS_GetGlobalObject(cx) slightly wrong in a case like
'otherWindow.formx.foo'?
You are using the caller window global rather than the callee window global as
the scope, no? But that is so minor for this hack.

Also, a void return for InstallGlobalNamespacePolluter is kinda ugly given all
the error return cases. I dunno, even if the existing caller is going to ignore
an error return code it stills seems bad to write a function that can fail
without informing the caller.

Nevertheless, I don't see any fatal errors in the code. I hope it either doesn't
make it into the tree or gets yanked soon.

Comment 6

[bobj]

> I hope it either doesn't make it into the tree or gets yanked soon.
No plan for this to land on the trunk.  Only for the 1.0 branch, so it should
wither away after this branch is no longer in use...

Comment 7

[bobj]

Based upon a meeting today to try to limit the effect of this, it was suggested
that we initialized the pref to be disabled everytime we startup.  I assume we
can just hard code this when we initialize the prefs?

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 100804 [details] [diff] [review]
Propsed fix (behind a pref, and works for https sites only, don't ask).

>+  if (!doc) {
>+    NS_ERROR("What? Someone installed a global scope polluter on a non-HTML"
>+             "document!");

Want a space between "non-HTML" and "document"?

>+  // We found a form, wrap it and put a reference to it on the global
>+  // object.
>+
>+  jsval val;
>+  rv = nsDOMClassInfo::WrapNative(cx, ::JS_GetGlobalObject(cx), result,
>+                                  NS_GET_IID(nsISupports), &val);

jband's got a point, don't you want obj here, or the window object on whose
prototype chain obj lies?

>+
>+  if (NS_FAILED(rv)) {
>+    return JS_TRUE;
>+  }
>+
>+  // Define a non-enumerable and non-permanent writable property on
>+  // the global object that referes to the form in the document.
>+
>+  PRBool ok = ::JS_DefineUCProperty(cx, obj, ::JS_GetStringChars(str),
>+                                    ::JS_GetStringLength(str), val, nsnull,
>+                                    nsnull, 0);

Here, you define the property in the polluter obj, which suggests you could use
obj above for the WrapNative call.

Does it make sense to #ifdef this much code, to avoid bloat and make it easier
to remove (slightly, via unifdef)?

Drivers are likely to want this only in a branch of the 1.0 branch, so it
doesn't become a permanent addition.

Did this patch have r= and sr= already, just not recorded in bugzilla?

/be

Comment 9

[Johnny Stenback (:jst)]

Yeah, the ::JS_GetGlobalObject(cx) thing was a cut n' paste goofup on my part,
it should definately be 'obj'. There are no reviews of this yet, I'll attach a
patch that fixes the above issue, then I'd appreciate reviews on this patch.

(I'm sure you understand that I can not talk about the reasons for doing this here.)

Comment 10

[bobj]

> I assume we can just hard code this when we initialize the prefs?
Probably better to re-initialize this pref in the embeddor and not in gecko?
Then it would be outside the scope of this bug.

Comment 11

[Johnny Stenback (:jst)]

Attachment: Fix jband's and brendan's issues.

Comment 12

[Jaime Rodriguez, Jr.]

Marking as nsbeta1+/topembed+, as this is needed by a major embedding customer
for the 1.0 branch only.

Comment 13

[Michael Dunn]

Related to:
http://bugscape.netscape.com/show_bug.cgi?id=20233

Comment 14

[Prashant Desale]

We think Entire DOM (other than events) would be needed to test on our part.

Comment 15

[Daniel Veditz [:dveditz]]

removing nsbeta1+ triage approval because I understood jst to say we DON'T want
this on the trunk for Buffy. If this is correct please mark nsbeta1-

Comment 16

[Johnny Stenback (:jst)]

This does affect specifically any code that uses undefined variables and relies
on them being undefined. Since the scope of this change is so broad any and all
DOM and JS testing that can be done should be done. And for testing that this
new functionlity works we need testcases that have forms with a name and/or an
id and we need to make sure that form is accessable in the global scope. From
what I tested here it works, but I have no real testcases that I can supply at
this point.

And of course, to even get this new code to execute, the pref
"dom.pollute_global_scope" must be set to true, and the site with the testcase
on it *must* be http_s_, http won't do it, you need https. If that's a problem
for the peope testing this, we can make this work on http (or on all protocols)
as well for testing purposes. Let me (or Roy who has an embedding build with
this change) know and we can easily change that.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Sync to the 1.0 branch

Comment 18

[Prashant Desale]

I have one simple basic testcase at https://bubblegum.mcom.com/desale/formtest.html

This testcase uses "https" protocol. onLoad of this test you would see alert
showing either "Test Passed" or "Test Failed" depending on if you can have
global acess to form.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: 1.0 diff with the #define goofup fixed

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 100946 [details] [diff] [review]
1.0 diff with the #define goofup fixed

This should be it for the 1.0 branch. I had to change what we pass to
WrapNative(), we can't pass |obj| there since XPConnect fails to find a scope
for that object, in stead I had to get the global object out of the window
we're resolving the property on, that will do the right thing and XPConnect is
happy.

Comment 21

[Jaime Rodriguez, Jr.]

doh! um ... how much would the latest mod effect our current testing? QA will be
testing with the patch attachment (id=100849)

Comment 22

[John Bandhauer]

Comment on attachment 100946 [details] [diff] [review]
1.0 diff with the #define goofup fixed

r=jband

Comment 23

[Johnny Stenback (:jst)]

Ouch. QA will need to test with either the first or the last patches, attachment
(id=100849) is no good :-(

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 100946 [details] [diff] [review]
1.0 diff with the #define goofup fixed

+  // Define a non-enumerable and non-permanent writable property on
+  // the global object that referes to the form in the document.

Typo: "referes"

sr=[e-mail] on the code quality, but drivers *do not* want the
code's bad effects to become part of the 1.0 branch (or of the trunk, of
course).

/be

Comment 25

[Jaime Rodriguez, Jr.]

wrt Comment #23 From Johnny Stenback, johnny and i chatted, and confirmed that
roy is using the latest patch in the embedding customer's private build.

Comment 26

[Jaime Rodriguez, Jr.]

Resolving this bug as WONTFIX. The patch will not be checked onto the 1.0
branch,nor the trunk. The embedding customer has been kind enough to make the
change locally in their tree. 

Many thanks to all who helped prep this patch for our customer. Much appreciated.

Comment 27

[Johnny Stenback (:jst)]

For the record, I went through these changes with rpotts as well, and he was
fine with this change at the code level. Thanks everyone for reviews n' all. I'm
glad we won't need to land this...

Comment 28

[Prashant Desale]

Well Shirley helped me & Siva set preferences so we can verify this bug  as well
as  bugscape bug 20038.
We found out that the testcase I provided is still failing on the builds
delievered to QA at 6.15 PM.
However Shirley performed test that is described at bugscape bug 20038 & She
said it is working fine & bug is verified.

Comment 29

[dsirnapalli]

-- verified bugscape bug#20038 and bugscape bug#20039. Both of them work as the
steps provided in them.