Closed Bug 739451 Opened 8 years ago Closed 8 years ago

Don't rely on XPConnect-magic for getting the owner window of a places view

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: bholley, Assigned: mano)

References

Details

Attachments

(1 file, 1 obsolete file)

Currently, the window is pulled off of aView with _getWindow. In the case of DOM nodes, we do the right thing (ownerDocument.defaultView). Otherwise though, we just return Cu.getGlobalForObject(aView). This has happened to work thus far, but there's no reason why it really should. XPCOM objects without a PreCreate hook have no 'home' compartment, so we just make a new XPCWrappedNative in each compartment. With compartment-per-global, we have multiple chrome compartments. So this becomes a problem.

Working on a patch now.
I think Mano planned to kill this after firefox 4, as the comment states, though I don't rememeber what the plan was.
Hm. So as it turns out, we don't actually need the view at all. And more importantly, all of these functions take nodes, from which we can find the document, and thus the window. So I think we can get rid of the view stuff entirely. Changing the bug title.
Summary: PlacesUIUtils.jsm needs explicit window arguments → PlacesUIUtils.jsm functions should get window references from nodes, not views
those are Places nodes, not dom nodes
Attached patch for-now (obsolete) — Splinter Review
Fixing this right still requires bug 619398...
Assignee: bobbyholley+bmo → mano
Attachment #609675 - Flags: review?
Summary: PlacesUIUtils.jsm functions should get window references from nodes, not views → Don't rely on XPConnect-magic for getting the owner window of a places view
Comment on attachment 609675 [details] [diff] [review]
for-now

Review of attachment 609675 [details] [diff] [review]:
-----------------------------------------------------------------

looks correct, though we have to check that all calls are properly passing into a view, and that's not the case
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.js#339

this is probably causing the test error that bholley notified me on IRC, since the test was exactly hitting this code path and complaining about a null window.
It also means this patch as is is regressing functionality.

Could you please fix that too?
Attachment #609675 - Flags: review?(mak77)
(In reply to Marco Bonardo [:mak] from comment #5)
> this is probably causing the test error that bholley notified me on IRC,
> since the test was exactly hitting this code path and complaining about a
> null window.

Yeah, I think I ran into that one too. But when I fixed it, there were still other failures. Do they appear for you as well?
(In reply to Marco Bonardo [:mak] from comment #5)
> looks correct, though we have to check that all calls are properly passing
> into a view, and that's not the case
> http://mxr.mozilla.org/mozilla-central/source/browser/components/places/
> content/places.js#339

also PlacesOrganizer.openSelectedNode(aEvent) is not always doing the right thing, looks like it sometimes passes in a bogus view.
(In reply to Marco Bonardo [:mak] from comment #7)

> also PlacesOrganizer.openSelectedNode(aEvent) is not always doing the right
> thing, looks like it sometimes passes in a bogus view.

Alright, cool. Can we get a fix? What's the next step? Are there other failures after that?

This is one of the last issues blocking compartment-per-global (or at least it's preventing us from finding further issues, since it messes up the rest of the browser-chrome run). We're trying to land c-p-g any day now, so I'd really appreciate if someone with knowledge of this code could sit down and hammer out all the necessary fixes to make the places tests pass (or tell me if XPConnect is doing something unexpected).
I fixed the callers, but I'm not sure what to do about Marco's last comment. It seems like an unrelated regression, which is likely to affect any tree that implements its view in js...

Marco, what do you see exactly? Is |view| null?
Oh, hrm, I guess what you see is just a result of the new ownerWindow property not being exposed after xpc-wrapping.
Attached patch ...Splinter Review
Regardless of this change, at some point we'll want to fix this treeview/placesview mess.
Attachment #609675 - Attachment is obsolete: true
Comment on attachment 612751 [details] [diff] [review]
...

tests pass on the git repository.
Attachment #612751 - Flags: review?(mak77)
Comment on attachment 612751 [details] [diff] [review]
...

Review of attachment 612751 [details] [diff] [review]:
-----------------------------------------------------------------

Does the trick for me as well, all tests seem to pass.
bholley, if you should find any other test issue, please file a new bug, and sorry if it took a while.
I see some (maybe expected) leaks of places.xul domwindows, but I don't see how this patch may cause those, if cpg is going to increase the number of leaked domwindows, you may want to do a try run first, or you may hit a permaorange against the mochitest-other leaks limit. But as I said, this patch is not at fault, could just be usual we leak those and I didn't notice before.
Attachment #612751 - Flags: review?(mak77) → review+
"you" was directed to Bholley, not to Mano, I suggest doing a mochitests-other tryrun before landing cpg in central.
https://hg.mozilla.org/mozilla-central/rev/690e77288e98
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 744189
Depends on: 762799
You need to log in before you can comment on or make changes to this bug.