368958 - Cross-window/openDialog object reference handling broken in post 2006-11-09 builds

Status: VERIFIED FIXED

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

Description

[TheSeer]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.0.9) Gecko/20061223 Fedora/1.0.7-0.6.fc6 pango-text SeaMonkey/1.0.7
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.0.9) Gecko/20061223 Fedora/1.0.7-0.6.fc6 pango-text SeaMonkey/1.0.7

Running the attached testcase results in a null-value for the object-property of the communication object on Firefox 2.0.0.1 while it works in FF 1.5.x, Seamonkey and all xulrunner builds i tried that date before 2006-11-10.

Reproducible: Always

Steps to Reproduce:
1. Load base.xul from testcase
2. Open dialog by clikcing the button
3. Click accept 
4. The textbox will hold the stringified (json) representation of the comObj
Actual Results:  
In FF 2.0.0.1: {"resMode":true,"resData":null,"debug":"12345"}
In Seamonkey (see build identifier): {"resMode":true,"resData":{"foo":"bar","blo":1234,"nest":{"blue":"abc"}},"debug":"12345"}


Expected Results:  
FF 2.0.0.1: Identical results as to seamonkey

In case it helps, I checkd various versions of xulrunner to trace down the problem 
to a specific build: It Works up to xulrunner/nightly/2006-11-09-08-mozilla1.8/ while it returns null for resData as of xulrunner/nightly/2006-11-10-08-mozilla1.8/ and later.

Comment 1

[TheSeer]

Attachment: testcase base window

Comment 2

[TheSeer]

Attachment: Dialog popup code for testcase

Comment 3

[TheSeer]

Attachment: stripped down version of json.js from json.org

This code is stripped down from the version that used to be available on json.org. All i did was remove the comments and the parse-method since for this testcase only the stringify method is needed.

Comment 4

[TheSeer]

Some more sidenotes - in case the help ;)

 - This testcase is run in chrome-context

 - It is interesting to note, that the string value assignment is passed just fine

 - Making the result-variable global did not have any effect while testing

Comment 5

[Boris Zbarsky [:bzbarsky]]

Need usable testcase; it's not clear what one does with the attached files to see the bug.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Given the regression range in comment 0:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-11-09+08&maxdate=2006-11-10+08&cvsroot=%2Fcvsroot

I'd guess at bug 353090 somehow... not sure how, though.

Comment 7

[TheSeer]

Attachment: Installable XPI for testcase

Installable XPI Version of the testcase.

After installation go to:

 1. Load chrome://comobj/content/base.xul
 2. Click on the button - a dialog should appear
 3. Click accept in the dialog
 4. The Textbox will hold the serialized representation of the comObj-variable

Comment 8

[TheSeer]

Attachment: Installable XPI for testcase - now with version 1.5 - 3.0+

Comment 9

[(not currently active) Ted Mielczarek]

resdata shows as null here, although when I tried to reproduce this (by writing my own testcase) yesterday it seemed to work.


Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1

Comment 10

[(not currently active) Ted Mielczarek]

Attachment: interesting test.xul

So, it seems like your object is there, it's just a confused object.  Your json.js is giving you null probably because it does |if(x)|.  If I write my own overly-cautious toString method, I can stringify it.  However, the object doesn't seem to have |hasOwnProperty|, and it won't auto-stringify.  The attached is essentially your test.xul, with a different stringify function.

Comment 11

[Daniel Veditz [:dveditz]]

David: please check out whether or not this is a regression from your fix for bug 353090.

If we're getting a mangled object after the dialog has touched it this could be freed memory -- bad times.

Comment 12

[Boris Zbarsky [:bzbarsky]]

So...

Having done no debugging but given the info in this bug, I'm guessing that this is indeed a regression from bug 353090.

At a guess, we're clearing the proto chain on the window, which includes Object.prototype.  So after that Object.prototype.toString doesn't so much exist, for example.

Then the json.js code does:

  switch (typeof x) {
    case 'object':
      if (x) {
        if (typeof x.toString != 'undefined') {
          // Code here that stringifies and returns
        }
      }
      e('null');
      return '';
  }

Since x.toString is in fact undefined, you get the results you get....

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, and just to make it clear, I doubt that there's freed memory involved in any way.  ;)

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Brendan thinks clearing up the prototype chain should be OK, except for Object.prototype.

Comment 15

[Boris Zbarsky [:bzbarsky]]

But then we can still leak via Object.prototype, no?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yep.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: possible 1.8 branch patch

This is what Brendan suggested.

I haven't tested this beyond making sure that the browser still starts up.

Comment 18

[Brendan Eich [:brendan]]

Only thought is to make a common subroutine (static helper function) to consolidate the repeated code.

/be

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I did the consolidation (which was actually rather complicated, and tied in with DOM agnostic) on the trunk.  I was hoping not to have to touch the branch too much more.

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #19)
> I did the consolidation (which was actually rather complicated, and tied in
> with DOM agnostic) on the trunk.  I was hoping not to have to touch the branch
> too much more.

Ok, no problem -- sorry I keep forgetting this is branch-only.

/be

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I can't tell if this regresses bug 353090 because I'm seeing leaks on http://www.google.com/ig on the 1.8 branch without this patch.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This patch does (on the 1.8 branch) make the steps in comment 7 show more text than they do without it, like they did before bug 353090 landed.

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 253708 [details] [diff] [review]
possible 1.8 branch patch

r+sr=jst

Comment 24

[Jay Patel [:jay]]

dbaron:  If the patch jst just reviewed is good enough for the 1.8 branch and we want it... please ask for approvals for landing?  Thanks!

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 253708 [details] [diff] [review]
possible 1.8 branch patch

This is conceptually a partial backout of bug 353090.

Before bug 353090 went in, we cleared scope on only the global object.  Afterwards we cleared scope on it, and then up to the top of the prototype chain.  This makes us stop one step before hitting the top of the prototype chain.

(Assuming the prototype chain is always of length > 1, i.e., that the global object itself always has a prototype, this is guaranteed to be in between the two patches.  I'll double-check this with jst...)

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 253708 [details] [diff] [review]
possible 1.8 branch patch

The regressing bug 353090 did not land on the 1.8.0 branch as far as I can tell. All things being equal I'd rather not tweak things there if we don't have to: not approved for 1.8.0 branch.

Approved to land on the 1.8 branch for 1.8.1.2, a=dveditz for drivers

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ah, oops, I can't keep track of branches.  I shouldn't have requested 1.8.0.10 approval.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, I actually discussed the issue in comment 25 with jst, and we decided it's safer to change the patch so we always JS_ClearScope on the global object's JS object itself, and then walk up the proto chain.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: trunk patch

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: trunk patch

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: revised 1.8 branch patch

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk and MOZILLA_1_8_BRANCH.

Comment 33

[Martijn Wargers (dead)]

Verified fixed on trunk, with a 2007-02-05 build, I get:
{"resMode":true,"resData":null,"debug":"12345"}
with the test extension, and with a 2007-02-06 build, I get:
{"resMode":true,"resData":{"foo":"bar","blo":1234,"nest":{"blue":"abc"}},"debug":"12345"}

The latter result is also what I get with the latest branch builds, so also verified that it works for the latest 1.8.1 build.