940688 - sdk/content/events has a race condition

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

I pointed this race condition out in our meeting today with a pop quiz https://etherpad.mozilla.org/3INuTUVwJq

Irakli you said that it didn't matter because one cannot add the same event listener twice, so in this context it was not an issue, but in face content/events is not adding the same listener, it is dynamically creating new functions to be listeners, so the race condition is in fact an issue, and you implemented this one.

I hope this is proof enough about how confusing this new pattern is, and demonstrates how we need something far better to be our WindowTracker replacement.

Perhaps bug 940550 is fine though I think that will need a JEP first.

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

That's not what I said, what I said was event listeners registered before window became interactive won't fire because inner document window actually going to change. If you disagree, please provide a test case so
we can fix it ?

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #1)
> That's not what I said, what I said was event listeners registered before
> window became interactive won't fire because inner document window actually
> going to change. If you disagree, please provide a test case so
> we can fix it ?

Ah weird, sorry about that.  So, do you mean that you think it is not possible to add event listeners before `DOMContentLoaded` has occurred? or that these particular event listeners cannot be added before `DOMContentLoaded` has occurred? and do you mean in general for html and xul this just cannot be done? or just xul? or just browser.xul?

As far as I know there should be no problem adding event listeners before the `DOMContentLoaded` event fires no matter what page is used (html or xul).  Here would be an example for html:

https://gist.github.com/erikvold/7591510

I don't have time to write the test for these small issues that I find all of the time, and it's overly distracting me at the moment, so since it doesn't block me I will have to leave it for someone else.  I think we should have the test anyhow though in case the implementation changes and add comments to sdk/content/events to make clear this what is going on if the current code actually has no problems.

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Erik I agree I don't think it worth spending either your or my time on this debates. As I mentioned in
weekly once my fix for Bug 843910 lands all of this code will become obsolete as all these necessary
events will be dispatched as observer notifications as well.

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

Hey Irakli, are you working on this one? if not could you please unassign yourself.

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

As a matter of fact I have landed platform changes to expose following events:

content-document-interactive - notification just before DOMContentLoaded
content-document-loaded - notification just before load
content-document-pagehide - notification just before pagehide
content-document-pageshow - notification just before pageshow

So we can replace all of that code with simple observer, but I don't actively work on it no. If you wanna take over feel free. Otherwise I'll keep that on my list.

Comment 6

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #5)
> As a matter of fact I have landed platform changes to expose following
> events:
> 
> content-document-interactive - notification just before DOMContentLoaded
> content-document-loaded - notification just before load
> content-document-pagehide - notification just before pagehide
> content-document-pageshow - notification just before pageshow
> 
> So we can replace all of that code with simple observer, but I don't
> actively work on it no. If you wanna take over feel free. Otherwise I'll
> keep that on my list.

Doesn't this depend on bug 946971?

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #6)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #5)
> > As a matter of fact I have landed platform changes to expose following
> > events:
> > 
> > content-document-interactive - notification just before DOMContentLoaded
> > content-document-loaded - notification just before load
> > content-document-pagehide - notification just before pagehide
> > content-document-pageshow - notification just before pageshow
> > 
> > So we can replace all of that code with simple observer, but I don't
> > actively work on it no. If you wanna take over feel free. Otherwise I'll
> > keep that on my list.
> 
> Doesn't this depend on bug 946971?

I don't think it does no, those observer notifications are dispatched although in E10S they are dispatched in content process. All one would need to do to react to them in chrome process is to set up a frame script that serializes / extracts data needed and sends it to a chrome message manager.

Also note that in most cases all the work that is done in response to those events would still happen in the content process so there will be no need of forwarding as far as I can tell.

Comment 8

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562