Closed Bug 610390 Opened 15 years ago Closed 15 years ago

Object wrappers no longer compare equal (among other things, window.wrappedJSObject != window.wrappedJSObject)

Categories

(Core :: XPConnect, defect, P2)

Product:
Core
Component:
XPConnect
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b8
Tracking Flags:
Tracking Status
blocking2.0 --- beta8+

People

(Reporter: kmag, Assigned: gal)

References

Details

(Whiteboard: [firebug-p1][compartments], fixed-in-tracemonkey)

Attachments

(2 files, 6 obsolete files)

Updated fix with gc problem fixed by gal, and merged to m-c [Checked in: Comment 17 & 18]
10.31 KB, patch
neil
: feedback-
Details | Diff | Splinter Review
browser-chrome testcase
1.07 KB, application/vnd.mozilla.xul+xml
Details
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101107 Firefox/4.0b8pre Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101107 Firefox/4.0b8pre The wrappedJSObjects returned by DOM nodes seem to return wrapped objects in such a way that each access returns a new wrapper which does not compare equal with any other wrappers for the same object. This problem extends to the wrappers returned by property accesses of the returned wrappers, such that the following always returns false: let (w = content.wrappedJSObject) w == w.parent || w.parent == w.parent; despite the fact that content == content.parent && content.parent == content.parent; This bug is in particular responsible for a conflict between Greasemonkey/Scriptish and Firebug: http://code.google.com/p/fbug/issues/detail?id=3586 http://github.com/greasemonkey/greasemonkey/issues/issue/1211 https://github.com/erikvold/scriptish/issues/issue/116 Reproducible: Always
Kris if you create a test case that shows this effect then Boris can helps us find a way to fix it. Please don't try to use the Firebug test as an example, because it relies on jsd which is know to be broken.
This is a known bug we intend to fix for beta 8. If you guys come up with a small test case, that's still useful.
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Ever confirmed: true
Priority: -- → P2
Target Milestone: --- → mozilla2.0b8
Assignee: general → nobody
Component: JavaScript Engine → XPConnect
OS: Linux → All
QA Contact: general → xpconnect
Hardware: x86_64 → All
Attached file Testcase 1 (obsolete) —
I don't know about Firebug's test, but the relevant code in Firebug that fails has nothing to do with the jsd. Test case attached.
gal is on this (I think).
Assignee: nobody → gal
Blocking.
blocking2.0: ? → beta8+
Whiteboard: [firebug-p1]
Whiteboard: [firebug-p1] → [firebug-p1][compartments]
Version: unspecified → Trunk
I don't see Gal on this. Who is?
We may not see it, but gal *is* on it :). He's got a patch, working out some wrinkles...
Attached patch patch (obsolete) — — Splinter Review
WIP, pretty close
Ignore the JS engine changes, bad parts got mixed in. Uploaded the patch to calm the nerves, working on a working version.
Attached patch patch ready for testing (obsolete) — — Splinter Review
Attachment #494829 - Attachment is obsolete: true
Attached patch patch with fixes (obsolete) — — Splinter Review
Attachment #494833 - Attachment is obsolete: true
Attached patch patch, passes test (obsolete) — — Splinter Review
Attachment #494849 - Attachment is obsolete: true
Attachment #494875 - Flags: review?(mrbkap)
Comment on attachment 494875 [details] [diff] [review] patch, passes test r=me with the null check for priv.
Attachment #494875 - Flags: review?(mrbkap) → review+
Attached patch patch with null check (obsolete) — — Splinter Review
pushed to try again
Pushed to tracemonkey. http://hg.mozilla.org/tracemonkey/rev/d31f58102b38
Whiteboard: [firebug-p1][compartments] → [firebug-p1][compartments], fixed-in-tracemonkey
This landed with the tm merge: http://hg.mozilla.org/mozilla-central/rev/d31f58102b38
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #494875 - Attachment is obsolete: true
Attachment #495046 - Attachment is obsolete: true
Flags: in-testsuite+
That test confuses me a bit. Does it mean to use an HTML iframe? If so, why does it have type="content"? That does nothing on HTML iframes... they always have the same type as their parent docshell.
Comment on attachment 495200 [details] [diff] [review] Updated fix with gc problem fixed by gal, and merged to m-c [Checked in: Comment 17 & 18] >+ <!-- test results are displayed in the html:body --> >+ <body xmlns="http://www.w3.org/1999/xhtml"> >+ <iframe type="content" >+ src="data:text/html,&lt;script&gt;var x=3&lt;/script&gt;" Unfortunately this iframe is an html iframe, and therefore doesn't support the type="content" attribute. Because the URL is a data: URL the contentWindow is therefore same-origin with the test and no wrappers ever get created. Maybe setting the src to one of our example test http URLs would work.
Attachment #495200 - Flags: feedback-
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #495200 - Attachment description: Updated fix with gc problem fixed by gal, and merged to m-c. → Updated fix with gc problem fixed by gal, and merged to m-c [Checked in: Comment 17 & 18]
So just to be clear, this was re-opened just because the test isn't testing the right thing here, not because of a known problem with this fix?
(In reply to comment #21) > So just to be clear, this was re-opened just because the test isn't testing the > right thing here, not because of a known problem with this fix? Actually I don't believe the fix works either.
I agree with neil. I still get content.wrappedJSObject != content.wrappedJSObject in a build containing the fix.
The patch worked for my test case. Kris, could you provide a test case I can run that fails?
gal: Adding xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" to the iframe in your testcase is enough to produce a failure for me.
Gal, did comment 25 help you in reproducing?
Trying #25.
comment 25 is a different bug, you change the iframe to a XUL iframe which is completely different from an HTML iframe. Have to debug. We will file a new bug depending on the results of that.
Blake and I tried various combinations of this. Just adding the line doesn't do anything (test still passes). We tried to make it truly a xul iframe, too, but that works as well. Please re-file with a test case that files and re-nominate.
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
I changed it to a XUL iframe because that seemed to have been your intent, given the type="content" attribute. As an HTML iframe, the wrappedJSObject property was null for me. However, it seems that the trigger was actually changing the src to "about:blank" rather than changing to a XUL iframe. In recent Firefox nightlies from the Tracemonkey tree, this is enough to produce a failure for me: <?xml version="1.0"?> <?xml-stylesheet href="chrome://global/skin" type="text/css"?> <window title="Mozilla Bug 610390" onload="var doc = document.getElementById('ifr').contentDocument; alert(doc.wrappedJSObject == doc.wrappedJSObject)" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> <iframe type="content" src="about:blank" id="ifr"/> </window>
(In reply to comment #28) > comment 25 is a different bug, you change the iframe to a XUL iframe which is > completely different from an HTML iframe. Have to debug. We will file a new bug > depending on the results of that. I wasn't aware that the original bug referred to an HTML iframe in any way... Kris, I don't suppose you can write, say, a browser-chrome test for this?
Sorry, I would have done before now, but I've been having trouble building mozilla-central with tests enabled recently. It turns out that it doesn't support BSD tar. The attached browser-chrome testcase produces a failure for me.
Attachment #488881 - Attachment is obsolete: true
reopen per c#32 test.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I keep seeing this bug opened and closed multiple times. Has anyone contacted Oracle about this? To me, it sounds like a bug inside the JavaScript assembler and not anything wrong with the Firefox source code. This would be similar to me setting X to 5 and Y to 5 in Microsoft Visual Basic .NET and trying to do something when X = Y (which they are) and the code never running. In this example, the problem would lie with Microsoft (as the assembler would have the bug, not my program code).
Oracle doesn't make a JavaScript assembler. They make a Java runtime, but Mozilla makes the JavaScript interpreter and compiler involved here.
Clearing beta8 blocking flag. Feel free to let me know if this should block the release, but is sounds like it was fixed in at least some cases.
blocking2.0: beta8+ → ?
I at least do feel it should still block; I agree its not a b8 blocker though now. However I surely am not familiar with this code enough to fix it, nor am I a driver for Firefox.
I believe this needs to block the release. Not only that, but it's an extension compat issue, so it should block the nearest beta we can manage...
Re-opening really doesn't help. This bug already landed and was shipped. Can we file a dependent bug with the new test case to reduce the confusion? (and that one should block b9)
I agree that this should block, and if it was fixed in some cases, I'm not sure what they are, although the patch did seem viable. I'll add that the machinery to make this work seems to be present, at least for a large subset of cases. While content.wrappedJSObject == content.wrappedJSObject returns false, XPCNativeWrapper(content.wrappedJSObject) == XPCNativeWrapper(content.wrappedJSObject) correctly returns true.
A new bug for comment 32 is needed - fixed bugs shouldn't be reopened unless their patches are backed out.
Status: REOPENED → RESOLVED
blocking2.0: ? → beta8+
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
In that case, can we establish exactly what the patch in question fixed? The testcase is useless, since it essentially tests (null == null). Is there a valid testcase that actually demonstrates the function of the change, and if not, shouldn't the patch be backed out? At the very least, the testcase should be removed.
The patch that landed includes a test that presumably showed a behavior change. That it may not be the same issue (or even related to) what this bug was originally filed about is unfortunate, but isn't particularly relevant at this point. "file a new bug" is absolutely not meant as a dismissal of the issue or a condemnation of your bug-reporting skills, it's just a plea for history-tracking sanity. Bugs are cheap - I'd be happy to file it if you don't want to.
Blocks: 620486
Blocks: 620487
No longer blocks: 620486
Depends on: 620486
My point is that the test in the patch doesn't show a behavior change. It passes with or without the patch in question. At least, in both cases, the wrappedJSObject in question is undefined, so it compares equal in both cases and thus the test always passes. I don't mind filing a new bug, but I'd like to establish what the fix for this bug actually accomplished first.
I filed a new bug (bug 620486).
> The patch that landed includes a test that presumably showed a behavior change. The presumption is wrong.
(In reply to comment #40) > XPCNativeWrapper(content.wrappedJSObject) == XPCNativeWrapper(content.wrappedJSObject) > > correctly returns true. Actually it returned true before the patch.
No longer blocks: 605001
No longer blocks: 620487
Andreas, could you explain (again) what the landed test actually checks, as it seems rather unclear to other people. Also, could you maybe update the summary of this bug, as it would seem your patch is unrelated to the initial issue, which you moved to bug 620486.
Serge: Please post comments regarding this bug on the NEW bug 620486. This bug has NOT been fixed yet and the only reason why it is posted twice (bug 610390) and (bug 610390) is because the fix did not land in time for the Firefox 4 Beta 8 release today (12/21/10)...which has already been delayed by three weeks and missed two other release dates. Mozilla is working on this bug for the Firefox 4 Beta 9 release now, which is why they created a duplicate bug filing (close 8 and open one for 9).
I meant OLD = (bug 610390) and NEW = (bug 620486). Typing error above.
(In reply to comment #49) > This bug has NOT been fixed yet and the only reason why it is posted twice (bug > 610390) and (bug 610390) is because the fix did not land in time Actually the reason for the second bug is that a fix did land (comment 18, comment 39) in time for beta 8, and it's confusing to deal with more than one issue per bug. The question is what it actually fixed, because it didn't fix the bug described.
No longer depends on: 620486
Depends on: 620486
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: