Use asynchronous getCharsetForURI in PlacesUtils.jsm

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: raymondlee, Assigned: raymondlee)

Tracking

Trunk
mozilla24
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

No longer blocks: 834543
Depends on: 834543
(Assignee)

Updated

6 years ago
Assignee: nobody → raymond
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
Posted patch WIP Patch (obsolete) — Splinter Review
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?
Flags: needinfo?(mak77)
(Assignee)

Updated

6 years ago
Attachment #724077 - Attachment is patch: true
(Assignee)

Comment 2

6 years ago
Posted patch WIP Patch v2 (obsolete) — Splinter Review
(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.
(Assignee)

Updated

6 years ago
Attachment #724559 - Attachment is patch: true
(Assignee)

Updated

6 years ago
Attachment #724077 - Attachment is obsolete: true
(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.
Flags: needinfo?(mak77)
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?
(Assignee)

Updated

6 years ago
Depends on: 852032
No longer depends on: 834543
(Assignee)

Comment 5

6 years ago
Will wait for bug 852032 PlacesBackups.jsm to land first.
(Assignee)

Comment 6

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

Updated

6 years ago
Attachment #724559 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Status: ASSIGNED → NEW
Raymond, what the status of this bug? Is it blocked on other work?
Flags: needinfo?(raymond)
(Assignee)

Updated

6 years ago
Blocks: 854925
(Assignee)

Updated

6 years ago
Blocks: 880922
(Assignee)

Updated

6 years ago
No longer blocks: 854925
(Assignee)

Comment 8

6 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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?
Flags: needinfo?(raymond)
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.
Flags: needinfo?(mak77)
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.
Flags: needinfo?(mak77)
(Assignee)

Comment 11

6 years ago
Posted patch v1 (obsolete) — Splinter Review
(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.
Attachment #761649 - Flags: feedback?(mak77)
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.
(Assignee)

Comment 13

6 years ago
Posted patch v2 (obsolete) — Splinter Review
(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
Attachment #761649 - Attachment is obsolete: true
Attachment #761649 - Flags: feedback?(mak77)
Attachment #761938 - Flags: review?(mak77)
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.
Attachment #761938 - Flags: review?(mak77) → review+
(Assignee)

Comment 15

6 years ago
(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
(Assignee)

Updated

6 years ago
Attachment #761938 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/82161c9015ab
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.