Open Bug 1351989 Opened 7 years ago Updated 2 years ago

Should be able to easily listen for loads on windowless browsers (windowless browsers should implement EventTarget and be their docShell's chromeEventHandler?)

Categories

(Core :: XPCOM, enhancement, P3)

enhancement

Tracking

()

Tracking Status
firefox55 --- affected

People

(Reporter: Gijs, Unassigned)

References

(Depends on 1 open bug)

Details

When we're using windowless browsers in HiddenFrame.jsm and webextensions code, we've noticed that we need to use some dark-ish magic (OK, I exaggerate) to listen for content loads. For iframes and browsers in the hidden window, you could generally use something like:

frame.addEventListener("DOMContentLoaded"/"load"/whatever, myFn, true);

and it would work.

the rv of createWindowlessBrowser doesn't have add/removeEventListener functions, its docshell has no chromeEventHandler... so we're having to write one-use web progress listeners, or use the global observer service to find out about inserted documents, or other hacky solutions.

It'd be nice if this was simpler.

Boris, would implementing nsIDOMEventTarget be the right solution and/or would it be difficult?

(background: bug 1351300 comment 3 and further)
Flags: needinfo?(bzbarsky)
We should probably consider migrating it to WebIDL and giving it a nicer/less-XPCOMMY interface, if we want to do this. But it would definitely make a lot of things much easier.
WindowlessBrowser would need to inherit from DOMEventTargetHelper and hence become cycle collected.  If we did that, we'd want to migrate to WebIDL indeed, because we do not want to keep supporting non-WebIDL things inheriting from WebIDL stuff like EVentTarget for much longer.

I'm a little shocked it's not already cycle-collected, but the things it holds to also are not... and its close() doesn't null out mBrowser.  This is all terribly leak-prone if anything inside that nsIWebBrowser ever gets a backref to the WindowlessBrowser somehow.  :(  We should fix some of that stuff somehow.

Anyway, I don't think implementing EventTarget here would be too complicated and seems to me like a reasonable solution.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #2)
> I'm a little shocked it's not already cycle-collected, but the things it
> holds to also are not... and its close() doesn't null out mBrowser.  This is
> all terribly leak-prone if anything inside that nsIWebBrowser ever gets a
> backref to the WindowlessBrowser somehow.  :(  We should fix some of that
> stuff somehow.

Hm. I'm not sure why we don't null out mBrowser or mContainer when close() is
called, but I think there was probably a reason. That said, once we call
Destroy(), the content window should be destroyed and there shouldn't be a way
for it to be involved in a reference cycle. Also, to be safe, almost
everywhere we use a windowless browser, we immediately null out the only
reference to it as soon as we call close().

Still, it would definitely be nice if it were cycle collected.
Hi Gijs. Can you help me find a nice home for this issue? I was thinking of Core :: DOM: Events :)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Vlad Bacia-Mociran [:VladB] from comment #4)
> Hi Gijs. Can you help me find a nice home for this issue? I was thinking of
> Core :: DOM: Events :)

The APIs here live on the appshell service, which is in XPFE which we no longer have a component for. It looks like the most recent relevant thing got filed under Core::XPCOM, so let's go with that I guess.
Component: Untriaged → XPCOM
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1353905
Priority: -- → P3
Summary: Should be able to easily listen for loads on windowless browsers (windowless browsers should implement nsIDOMEventTarget.idl and be their docShell's chromeEventHandler?) → Should be able to easily listen for loads on windowless browsers (windowless browsers should implement EventTarget and be their docShell's chromeEventHandler?)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.