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

VERIFIED FIXED

Status

()

Core
DOM
--
major
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: TheSeer, Assigned: dbaron)

Tracking

({regression, verified1.8.1.2})

1.8 Branch
x86
Linux
regression, verified1.8.1.2
Points:
---
Bug Flags:
blocking1.8.1.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(4 attachments, 6 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 253591 [details]
testcase base window
(Reporter)

Comment 2

11 years ago
Created attachment 253592 [details]
Dialog popup code for testcase
(Reporter)

Comment 3

11 years ago
Created attachment 253593 [details]
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.
(Reporter)

Comment 4

11 years ago
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

Need usable testcase; it's not clear what one does with the attached files to see the bug.
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.
Assignee: nobody → general
Blocks: 353090
Component: General → DOM
Keywords: regression
Product: Firefox → Core
QA Contact: general → ian
Version: unspecified → 1.8 Branch
Flags: blocking1.9?
Flags: blocking1.8.1.2?
(Reporter)

Comment 7

11 years ago
Created attachment 253634 [details]
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
Attachment #253591 - Attachment is obsolete: true
Attachment #253592 - Attachment is obsolete: true
Attachment #253593 - Attachment is obsolete: true
(Reporter)

Comment 8

11 years ago
Created attachment 253637 [details]
Installable XPI for testcase - now with version 1.5 - 3.0+
Attachment #253634 - Attachment is obsolete: true
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
Created attachment 253639 [details]
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.
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.
Assignee: general → dbaron
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: need investigation (dbaron)
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....
Oh, and just to make it clear, I doubt that there's freed memory involved in any way.  ;)
(Assignee)

Comment 14

11 years ago
Brendan thinks clearing up the prototype chain should be OK, except for Object.prototype.
But then we can still leak via Object.prototype, no?
(Assignee)

Comment 16

11 years ago
Yep.
(Assignee)

Comment 17

11 years ago
Created attachment 253708 [details] [diff] [review]
possible 1.8 branch patch

This is what Brendan suggested.

I haven't tested this beyond making sure that the browser still starts up.
Only thought is to make a common subroutine (static helper function) to consolidate the repeated code.

/be
(Assignee)

Comment 19

11 years ago
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.
(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
Flags: blocking1.8.1.2? → blocking1.8.1.2+
Whiteboard: need investigation (dbaron) → need reviews (bz? jst?)
(Assignee)

Comment 21

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

Comment 22

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

Updated

11 years ago
Attachment #253708 - Flags: superreview?(jst)
Attachment #253708 - Flags: review?(jst)
(Assignee)

Updated

11 years ago
Whiteboard: need reviews (bz? jst?) → [patch]need reviews (bz? jst?)
Comment on attachment 253708 [details] [diff] [review]
possible 1.8 branch patch

r+sr=jst
Attachment #253708 - Flags: superreview?(jst)
Attachment #253708 - Flags: superreview+
Attachment #253708 - Flags: review?(jst)
Attachment #253708 - Flags: review+

Comment 24

11 years ago
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!
(Assignee)

Comment 25

11 years ago
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...)
Attachment #253708 - Flags: approval1.8.1.2?
Attachment #253708 - Flags: approval1.8.0.10?
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
Attachment #253708 - Flags: approval1.8.1.2?
Attachment #253708 - Flags: approval1.8.1.2+
Attachment #253708 - Flags: approval1.8.0.10?
Attachment #253708 - Flags: approval1.8.0.10-
(Assignee)

Comment 27

11 years ago
Ah, oops, I can't keep track of branches.  I shouldn't have requested 1.8.0.10 approval.
(Assignee)

Updated

11 years ago
Whiteboard: [patch]need reviews (bz? jst?) → [patch]
(Assignee)

Comment 28

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

Comment 31

11 years ago
Created attachment 254117 [details] [diff] [review]
revised 1.8 branch patch
Attachment #253708 - Attachment is obsolete: true
Attachment #254116 - Attachment is obsolete: true
(Assignee)

Comment 32

11 years ago
Fix checked in to trunk and MOZILLA_1_8_BRANCH.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.2
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Flags: blocking1.9?
Keywords: fixed1.8.1.2 → verified1.8.1.2

Updated

11 years ago
Blocks: 369717

Updated

11 years ago
No longer blocks: 369717
You need to log in before you can comment on or make changes to this bug.