846644 - Use asynchronous getCharsetForURI in PlacesUtils.jsm

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Raymond Lee [:raymondlee]]

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#1540

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: WIP Patch

I have a question regarding to browser/components/places/tests/browser/browser_drag_bookmarks_on_toolbar.js.  The test simulates a drag action and have a 'dragstart' listener to capture that action and do some checks.  

In the browser/components/places/content/browserPlacesViews.js, the handleEvent() would invoked when a drag start event happens.  

The action sequence would be: 
this._onDragStart(event) -> this._controller.setDataTransfer(event) -> PlacesUtils.wrapNode() -> PlacesUtils.serializeNodeAsJSONToOutputStream() -> PlacesUtils.history.getCharsetForURI().

Since getCharsetForURI needs to be async, all its callers would also be async.  Therefore, the test in browser_drag_bookmarks_on_toolbar.js wouldn't get the correct data to do checks when 'dragstart' event is fired as this._controller.setDataTransfer(event) sets the data transfer asynchronously.

Any suggestions to solve this?

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: WIP Patch v2

(In reply to Raymond Lee [:raymondlee] from comment #1)
> Created attachment 724077 [details] [diff] [review]
> WIP Patch
> 
> I have a question regarding to
> browser/components/places/tests/browser/browser_drag_bookmarks_on_toolbar.js.
> The test simulates a drag action and have a 'dragstart' listener to capture
> that action and do some checks.  
> 
> In the browser/components/places/content/browserPlacesViews.js, the
> handleEvent() would invoked when a drag start event happens.  
> 
> The action sequence would be: 
> this._onDragStart(event) -> this._controller.setDataTransfer(event) ->
> PlacesUtils.wrapNode() -> PlacesUtils.serializeNodeAsJSONToOutputStream() ->
> PlacesUtils.history.getCharsetForURI().
> 
> Since getCharsetForURI needs to be async, all its callers would also be
> async.  Therefore, the test in browser_drag_bookmarks_on_toolbar.js wouldn't
> get the correct data to do checks when 'dragstart' event is fired as
> this._controller.setDataTransfer(event) sets the data transfer
> asynchronously.
> 
> Any suggestions to solve this?

1) This patch still has the above issue.
2) I've made some changes to files under services/sync/tps/extensions/tps.  I have tried to follow https://developer.mozilla.org/en-US/docs/TPS to run some tests but without luck.

Comment 3

[Marco Bonardo [:mak]]

(In reply to Raymond Lee [:raymondlee] from comment #1)
> Since getCharsetForURI needs to be async, all its callers would also be
> async.  Therefore, the test in browser_drag_bookmarks_on_toolbar.js wouldn't
> get the correct data to do checks when 'dragstart' event is fired as
> this._controller.setDataTransfer(event) sets the data transfer
> asynchronously.

There is even more, as far as I know we cannot set dataTransfer asynchronously so the solution there may be far more complicated than expected.
I think for the D&D case we may just skip the charset, it's not really needed afaict. Though this means the solution may be more complicated if the serialization is async, thus I suggest filing a bug apart, and please cc Mano to it.

Comment 4

[Marco Bonardo [:mak]]

looking at the patch, there are many changes that look scary and huge, especially regarding the backups.
reviewing a 150KB patch like this is basically impossible, we should split the problem in parts and coordinate on them before coding thousands lines of code.
For exampel doesn't make sense to change all of the backups code until we have bug 822200, would mean re-doing the same things twice.

So, could you please report about each problem caused by async charset and the touched files for each of them?

Comment 5

[Raymond Lee [:raymondlee]]

Will wait for bug 852032 PlacesBackups.jsm to land first.

Comment 6

[Raymond Lee [:raymondlee]]

bug 854761 comment 6 mentioned to keep wrapNode synchronous for now, therefore, we should not change synchronous getCharsetForURI in the _serializeNodeAsJSONToOutputStream() for now.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Raymond, what the status of this bug? Is it blocked on other work?

Comment 8

[Raymond Lee [:raymondlee]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #7)
> Raymond, what the status of this bug? Is it blocked on other work?

PU_wrapNode -> PU__serializeNodeAsJSONToOutputStream -> PlacesUtils.history.getCharsetForURI.  If want to resolve this bug, we have to make wrapNode async.  Mak mentioned in bug 854761 comment 6 that since wrapNode is totally synchronous and this should probably keep using the synchronous version for now, therefore, I didn't go further with this bug.

This bug is just blocked on bug 880922 (Add depreciate warning to both SetCharsetForURI and GetCharsetForURI in nsNavHistory.cpp)

Any suggestions what we should with sync wrapNode method?

Comment 9

[Marco Bonardo [:mak]]

it's unfortunately a whac-a-mole, where trying to replace some synchronous behavior brings over another synchronous behavior to fix.
wrapNode is usedby d&d and copy, both are synchronous by API definitions (at the beginning of the operation you must populate a transfer object). But actually, I'm evaluating if we could just stop supporting charset in copy and drag operations, for a very simple reason, charset is defined as a page annotation, the page record and the annotation record are the same, making a copy will copy the annotation, overwriting the already existing one, a pointless operation.
The only relevant case for copy would be copying across 2 browsers using Places, quite an edge case, we may not care.
The remaining issue is cut/paste or move (where this is done through remove+create), I must verify that the code for these doesn't remove the entry before recreating it, if this is not the case then we are pretty much done.
I'll set a needinfo on myself to verify that.

Comment 10

[Marco Bonardo [:mak]]

Well, looks like we stopped doing dumb things like remove and create when I rewrote the cut command, so I think we can do what I suggested, just remove the charset support from wrapnode, replace it with a comment explaining the fact page annotations are bound to the page and thus there's no need to store them apart and set them again when copying or moving the page.
If we should ever find a case where we still do the dumb remove+create thing, that should be considered a bug and also a performance hit.

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: v1

(In reply to Marco Bonardo [:mak] from comment #10)
> Well, looks like we stopped doing dumb things like remove and create when I
> rewrote the cut command, so I think we can do what I suggested, just remove
> the charset support from wrapnode, replace it with a comment explaining the
> fact page annotations are bound to the page and thus there's no need to
> store them apart and set them again when copying or moving the page.
> If we should ever find a case where we still do the dumb remove+create
> thing, that should be considered a bug and also a performance hit.

The charset code lives in _serializeNodeAsJSONToOutputStream() in PlacesUtils.jsm and the _serializeNodeAsJSONToOutputStream() is still used by some of the addons via serializeNodeAsJSONToOutputStream() with a depreciate warning.  Therefore, I can't just removed the charset code there and add a input param to disable it.

Comment 12

[Marco Bonardo [:mak]]

well, You can replace that deprecated getCharsetForURI with a simple PlacesUtils.annotations.getPageAnnotation(aURI, PlacesUtils.CHARSET_ANNO); for now, so we can proceed with cleaning up the API.

Comment 13

[Raymond Lee [:raymondlee]]

Attachment: v2

(In reply to Marco Bonardo [:mak] from comment #12)
> well, You can replace that deprecated getCharsetForURI with a simple
> PlacesUtils.annotations.getPageAnnotation(aURI, PlacesUtils.CHARSET_ANNO);
> for now, so we can proceed with cleaning up the API.

Done

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 761938 [details] [diff] [review]
v2

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

::: browser/components/places/tests/unit/test_leftpane_corruption_handling.js
@@ +129,5 @@
> +      let leftPaneJSON = yield folderToJSON(gLeftPaneFolderId);
> +      do_check_true(compareJSON(gReferenceJSON, leftPaneJSON));
> +      do_check_eq(PlacesUtils.bookmarks.getItemTitle(gFolderId), "test");
> +      // Go to next test.
> +      do_timeout(0, run_next_test); 

trailing space

::: toolkit/components/places/PlacesUtils.jsm
@@ +562,5 @@
> +        // Page annotations are bound to the page and thus there's no need to
> +        // store them apart and set them again when copying or moving the page.
> +        // Hence, we remove the charset support when serializing node to output.
> +        // If we should ever find a case where we still do the dumb remove+create
> +        // thing, that should be considered a bug and also a performance hit.

well, but we are still reading the charset annotation, just through a different API, so this comment is currently not needed.

Comment 15

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

(In reply to Marco Bonardo [:mak] from comment #14)
> Comment on attachment 761938 [details] [diff] [review]
> v2
> 
> Review of attachment 761938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/places/tests/unit/test_leftpane_corruption_handling.js
> @@ +129,5 @@
> > +      let leftPaneJSON = yield folderToJSON(gLeftPaneFolderId);
> > +      do_check_true(compareJSON(gReferenceJSON, leftPaneJSON));
> > +      do_check_eq(PlacesUtils.bookmarks.getItemTitle(gFolderId), "test");
> > +      // Go to next test.
> > +      do_timeout(0, run_next_test); 
> 
> trailing space

Done

> 
> ::: toolkit/components/places/PlacesUtils.jsm
> @@ +562,5 @@
> > +        // Page annotations are bound to the page and thus there's no need to
> > +        // store them apart and set them again when copying or moving the page.
> > +        // Hence, we remove the charset support when serializing node to output.
> > +        // If we should ever find a case where we still do the dumb remove+create
> > +        // thing, that should be considered a bug and also a performance hit.
> 
> well, but we are still reading the charset annotation, just through a
> different API, so this comment is currently not needed.

Done

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=c4e8f97a117f

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82161c9015ab

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/82161c9015ab