482917 - [HTML5] Make HTML5 parsing not regress stream data availability to extensions

Status: RESOLVED WONTFIX

(Core :: DOM: HTML Parser, defect, P5)

Description

[Henri Sivonen (:hsivonen)]

Make sure the HTML5 parser doesn't regress bug 265334.

Comment 1

[Henri Sivonen (:hsivonen)]

jst, is there any indication of the API introduced in bug 265334 still being needed?

Comment 2

[Johnny Stenback (:jst)]

IIRC that API was added for google desktop, and AFAIK they still use it, but I could be wrong. The way to test would be to install google desktop (on windows) and see if something gets registered so that Firefox ends up calling that API while browsing.

Comment 3

[Henri Sivonen (:hsivonen)]

Finding out if Google Desktop still uses this will probably involve putting some printfs in Firefox 3.5 branch and Firefox 3.6 branch on one day, pushing to try and running the builds on a Windows VM the next day. If Google Desktop no longer uses this functionality, we can probably just remove the API trivially.

If Google Desktop still uses this API, supporting it the old way won't work, since part of the parser is no longer on the main thread. This would probably lead to discussing alternatives with the Google Desktop engineers, and it's hard to guess how long that would take.

Comment 4

[Johnny Stenback (:jst)]

As a first step we could probably simply make a copy of the data and send it back to the main thread and use the existing API there, and we'd of course only do that if anyone's listening for these notifications. And once we have a feel for how commonly we run on systems where this ends up being used we can decide on whether or not that's good enough, or if we need a better API going forward.

Comment 5

[Henri Sivonen (:hsivonen)]

(In reply to comment #2)
> IIRC that API was added for google desktop, and AFAIK they still use it, but I
> could be wrong. The way to test would be to install google desktop (on windows)
> and see if something gets registered so that Firefox ends up calling that API
> while browsing.

I added a diagnostic printf to the 1.9.1 branch and the 1.9.2 branch on the try server. 

Then I installed Firefox 3.5.8 and the 1.9.1 "Shiretoko" build with printf on a Windows XP VM running Google Desktop. I verified that Google Desktop found items in the Firefox 3.5 browsing history. Google Desktop put its files in the Firefxo 3.5 components directory but not into the corresponding Shiretoko directory. I copied the GoogleDesktopMozilla* files to the Shiretoko components directory and launched the app. A parser data listener was installed.

I then uninstalled both Firefox 3.5.8 and Shiretoko without removing the third-party components left behind. 

I then installed Firefox 3.6 in the default location (i.e. over the third-party components left behind previously). Just in case, I uninstalled Google Desktop and reinstalled it with Firefox 3.6 already in place and set as the default browser. Google Desktop no longer picked up the browsing history from Firefox, so I didn't proceed to examining the parser data listeners.

Since Firefox 3.6 no longer loads 3rd-party libs from the components directory, it seems that Google Desktop (5.9.0911.03589-fi-pb) is incompatible with Firefox 3.6 or later.

(In reply to comment #4)
> As a first step we could probably simply make a copy of the data and send it
> back to the main thread and use the existing API there, and we'd of course only
> do that if anyone's listening for these notifications.

In that case, the order of network-originating data and document.written data wouldn't be guaranteed. Also, in a naive implementation, the same data could be sent multiple times if the stream is parsed multiple times due to speculation failures.

It seems to me that addressing these problems isn't worthwhile unless we have a solid use case for an extension listening to the raw markup decoded to UTF-16 with document.written content included in the parsed order.

For example, if the actual use case is doing full text indexing without paying attention to the tags, it would be simpler (and code-wise less ugly) to send the contents of text node from the tree op executor to the extension. On the other hand, there's already another API for listening to the bytes from the network (without document.written stuff).

Now that Google Desktop doesn't support Firefox 3.6, is there any known extension that works with Firefox 3.6 and that would no longer work if we dropped this API?

Comment 6

[Henri Sivonen (:hsivonen)]

Suspected users of the API include Beagle (according to shaver) and roboforms (according to timeless).

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to comment #5)
> In that case, the order of network-originating data and document.written data
> wouldn't be guaranteed. Also, in a naive implementation, the same data could be
> sent multiple times if the stream is parsed multiple times due to speculation
> failures.

I guess this could be addressed by putting the data in tree ops in which case the mechanisms for dealing with the order of the tree ops would also order this data correctly.

Comment 8

[Henri Sivonen (:hsivonen)]

Attachment: Untested patch

Notes:
 * The data passed to listeners has had CRLF normalization performed on it, because exposing the data this way was simpler. This shouldn't break any of the use cases.
 * The old parser assumed that the extensions that want to use this API have registered themselves in the right place before the the old parser module loads. This patch assumes--completely without testing--that the extensions have registered themselves in the right place by the time the *layout* module loads.

Comment 9

[Henri Sivonen (:hsivonen)]

As far as I can tell, the Beagle Firefox extension, version 1.1.2, neither has nsIUnicharStreamListener in its source code nor registers a parser data listener.

Comment 10

[Henri Sivonen (:hsivonen)]

I installed RoboForm on a Windows XP VM. Even though a .js file of its Firefox extensions seems to implement QI for nsIUnicharStreamListener (but not the actual methods), I don't see the extension actually getting used as a parser data listener in either the old or the new parser.

As it stands, I am not aware of any Firefox extension that I could use to test the patch on this patch.

Comment 11

[Henri Sivonen (:hsivonen)]

"the patch on this bug", I meant to say.

Comment 12

[Henri Sivonen (:hsivonen)]

Demoting to P5 to take this off P2 queries because this bug is now suspended until users of the API show up.

Comment 13

[Henri Sivonen (:hsivonen)]

This looks like a pretty clear WONTFIX now. I have seen no complaint about this.