Bug 103702 – PluginDocument fires didFinishDocumentLoadForFrame upon receiving initial bytes instead of when load completes

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Tim Horton]

PluginDocumentParser::appendBytes() was made to call finish() in http://trac.webkit.org/changeset/14838. This is a bit odd because it basically means that onload and all the other load-y events get called immediately when the first bytes arrive, instead of when the document's main resource is actually finished loading.

However, DocumentWriter::end() already calls finish() at a more appropriate time. Reading http://trac.webkit.org/changeset/114062 makes me assume that at some point in the past (and likely in the case of r14838), finish() would not get called in the case of a main resource load, but it is now.

I'm having trouble doing sufficient archaeology to determine when that change occurred, but I don't think it matters.

I'd like to remove that call to finish() if there are no objections.

<rdar://problem/12762534>

[Brady Eidson]

(In reply to comment #0)
> PluginDocumentParser::appendBytes() was made to call finish() in http://trac.webkit.org/changeset/14838. This is a bit odd because it basically means that onload and all the other load-y events get called immediately when the first bytes arrive, instead of when the document's main resource is actually finished loading.
> 
> However, DocumentWriter::end() already calls finish() at a more appropriate time. Reading http://trac.webkit.org/changeset/114062 makes me assume that at some point in the past (and likely in the case of r14838), finish() would not get called in the case of a main resource load, but it is now.
> 
> I'm having trouble doing sufficient archaeology to determine when that change occurred, but I don't think it matters.
> 
> I'd like to remove that call to finish() if there are no objections.

Here's why I hesitate to support this change - I'm not sure it matters!  Is there any difficulty we're running in to as a result here?  Do PluginDocuments even *have* script that observes this?

Thinking logically about what a PluginDocument is... it's a template of HTML built in to the engine which instantiates a Plugin with the main resource data.  Especially in a world of asynchronous plugin initialization I'm not yet convinced this is *wrong*.

Hearing any symptom it causes could quickly change my mind.

[Tim Horton]

(In reply to comment #1)
> (In reply to comment #0)
> > PluginDocumentParser::appendBytes() was made to call finish() in http://trac.webkit.org/changeset/14838. This is a bit odd because it basically means that onload and all the other load-y events get called immediately when the first bytes arrive, instead of when the document's main resource is actually finished loading.
> > 
> > However, DocumentWriter::end() already calls finish() at a more appropriate time. Reading http://trac.webkit.org/changeset/114062 makes me assume that at some point in the past (and likely in the case of r14838), finish() would not get called in the case of a main resource load, but it is now.
> > 
> > I'm having trouble doing sufficient archaeology to determine when that change occurred, but I don't think it matters.
> > 
> > I'd like to remove that call to finish() if there are no objections.
> 
> Here's why I hesitate to support this change - I'm not sure it matters!  Is there any difficulty we're running in to as a result here?  Do PluginDocuments even *have* script that observes this?
> 
> Thinking logically about what a PluginDocument is... it's a template of HTML built in to the engine which instantiates a Plugin with the main resource data.  Especially in a world of asynchronous plugin initialization I'm not yet convinced this is *wrong*.
> 
> Hearing any symptom it causes could quickly change my mind.

Please read the Radar.

[Tim Horton]

(In reply to comment #2)
> > Hearing any symptom it causes could quickly change my mind.
> 
> Please read the Radar.

For those who can't, the symptom is basically that clients who listen to didFinishDocumentLoadForFrame get the notification way too early.

[Tim Horton]

Created attachment 176905 [details]
patch

[Alexey Proskuryakov]

The patch looks good to me, but I'd like to better understand whether this only affects DOMContentLoaded, or also the load event.

[Tim Horton]

By experiment, both DOMContentLoaded and onload now fire when the main resource finishes loading instead of immediately (I confirmed they both fired immediately before, too).

[Tim Horton]

Technically, this is about didFinishDocumentLoadForFrame, not DOMContentLoaded. Also, we really ideally wouldn't move DOMContentLoaded - the DOM is actually ready immediately upon PluginDocument construction. However, we're not convinced there's actually a way to observe DOMContentLoaded on a PluginDocument, so it likely doesn't matter.

[Alexey Proskuryakov]

Comment on attachment 176905 [details]
patch

r=me

I think that the new behavior is correct for the load event, but not logical for DOMContentLoaded.

However, the purpose of this bug is to get client delegates at the right time, and timing of DOM events doesn't matter nearly as much. Also, I'm not sure if the change of behavior for DOMContentLoaded is even observable.

Please update the bug title to track what we actually want to fix in Bugzilla and in ChangeLog.

[Tim Horton]

(In reply to comment #8)
> (From update of attachment 176905 [details])
> r=me
> 
> I think that the new behavior is correct for the load event, but not logical for DOMContentLoaded.
> 
> However, the purpose of this bug is to get client delegates at the right time, and timing of DOM events doesn't matter nearly as much. Also, I'm not sure if the change of behavior for DOMContentLoaded is even observable.

Indeed. Thanks, Alexey!

http://trac.webkit.org/changeset/136289