870262 - Implement :visited selector in B2G

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect, P1)

Description

[sync-1]

AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.019.094
 Firefox os  v1.0.1
 Mozilla build ID:20130502070201
 
 
 +++ This bug was initially created as a clone of Bug #451206 +++
 
 DEFECT DESCRIPTION:
 
  The ":visited" selector does not work properly in the browser. When you selected a link and back the link does not change to the color.
 
  REPRODUCING PROCEDURES:
 
 1.- Insert the proper sim card in the device. With data available
 2.- Turn on the device.
 3.- Open the browser.
 4.- Go to the following link
 ""http://testing.handsets.es/browser/Pruebas_XHTMLMP_SIN_INFORME/PRUEBAS_CSS/RAMA_SELECTORES/wml2_fun_0201_5.11.2.xhtml"".
 5.- Select Link N1 and bakc check if the link has a different color.
 
 
  EXPECTED BEHAVIOUR:
 
  The link1 should be marked to green color after to go and go back from it.
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 Low
  REPRODUCING RATE:
 100%
  For FT PR, Please list reference mobile's behavior:
 
 ++++++++++ end of initial bug #451206 description ++++++++++
 
 
 
 CONTACT INFO (Name,Phone number):
 
  DEFECT DESCRIPTION:
 
  REPRODUCING PROCEDURES:
 
  EXPECTED BEHAVIOUR:
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

triage with tef: not a blocker

Comment 3

[Ben Francis [:benfrancis]]

This is because global history is turned off. I think there's a bug filed to implement this feature somewhere but I can't find it.

Comment 4

[Ben Francis [:benfrancis]]

Maybe this isn't a dupe then. Let's use this bug.

I'm really not sure how we should go about implementing this in B2G without turning global history back on.

Comment 6

[Jason Smith [:jsmith]]

Not a blocker - this is a feature request.

Comment 7

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Ben Francis [:benfrancis] PTO until 11th August from comment #4)
> Maybe this isn't a dupe then. Let's use this bug.
> 
> I'm really not sure how we should go about implementing this in B2G without
> turning global history back on.

:willyaranda showed me some related info. This had been modified due privacy issues [1].
Related bug 147777

[1] https://hacks.mozilla.org/2010/03/privacy-related-changes-coming-to-css-vistited/

Comment 8

[Guillermo López :willyaranda (probably SLOW response)]

I was checking this yesterday after talking with Beatriz and, can we enable the global history for the mozbrowser iframes?

I know this is possible for <xul browser> elements to set the attribute "disableglobalhistory"[1].

That will mean that gecko will store the pages visited on the mozbrowser iframes in the global history, but I think that this will duplicate what the browser (system?) do right now.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/disableglobalhistory

Comment 9

[Ben Francis [:benfrancis]]

(In reply to Guillermo López :willyaranda from comment #8)
> That will mean that gecko will store the pages visited on the mozbrowser
> iframes in the global history, but I think that this will duplicate what the
> browser (system?) do right now.

Yes, B2G stores browser history in a DataStore/IndexedDB. Turning on global history would duplicate this and could not be cleared by the user. That's why it wasn't turned on in the first place, which is also the reason :visited selectors don't work.

Comment 10

[[:fabrice] Fabrice Desré]

Well, there is also the fact that we disabled the "places" database used on desktop, and that's what the link coloring mechanism uses in gecko. We need to rewrite a backend for that like firefox for android did.

If we want to keep it in sync with gaia (and I think we want), we likely want to write some glue between gecko and gaia so that gaia can inform gecko of visited links.

Comment 11

[Albert [:albert]]

Attachment: Gaia patch

Comment 12

[Albert [:albert]]

Attachment: Gecko patch (WIP)

Comment 13

[Albert [:albert]]

Attachment: Gecko patch (WIP)

The patch enables :visited for the reported URL and some links, still working on make it functional for all links.

Comment 14

[Albert [:albert]]

Attachment: Gecko patch

The patch adds a nsB2GHistory implementing the history service, so when gaia reports a visited site it is stored in an array. Then, when a link checks if it is visited history only has to find the uri in the visited sites array.

I used an array as a cache in order to avoid sending a mozChromeEvent and a mozContentEvent every time a link is painted.

Comment 15

[Albert [:albert]]

Comment on attachment 8510096 [details] [review]
Gaia patch

Gaia sends a mozContentEvent for each visited site to notify gecko history.

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 8512742 [details] [diff] [review]
Gecko patch

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

Albert, thanks for working on that!

I have a few questions:
- SendReadVisited() is a sync IPC call. When does that happen, and what is the cost? I would not like to regress eg. app startup time because of that.
- What is the persistence story here? I may miss something, but it looks like we're not persisting anything to disk, so we start with an empty history after each reboot?

In general, having comments in the code would help a lot!

::: dom/ipc/ContentChild.cpp
@@ +1865,5 @@
> +    NS_ABORT_IF_FALSE(b2gHistory,
> +                 "We have no b2gHistory in the Chrome process !");
> +
> +//    nsTArray<nsString>& visited = b2gHistory->getVisited();
> +//    visited.AppendElement(aVisited);

nit: remove commented code

::: dom/ipc/PContent.ipdl
@@ +634,5 @@
>      // nsIPermissionManager messages
>      sync ReadPermissions() returns (Permission[] permissions);
>  
> +    sync ReadVisited() returns (nsString[] visited);
> +    

nit: whitespace

::: embedding/browser/nsWebBrowser.cpp
@@ -311,5 @@
>  
>  NS_IMETHODIMP nsWebBrowser::EnableGlobalHistory(bool aEnable)
>  {
>      NS_ENSURE_STATE(mDocShell);
> -    

cleanup this change.

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 8510096 [details] [review]
Gaia patch

Clearing review for now.

Comment 18

[Albert [:albert]]

(In reply to Fabrice Desré [:fabrice] from comment #16)
> Comment on attachment 8512742 [details] [diff] [review]
> Gecko patch
> 
> Review of attachment 8512742 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Albert, thanks for working on that!
> 
> I have a few questions:
> - SendReadVisited() is a sync IPC call. When does that happen, and what is
> the cost? I would not like to regress eg. app startup time because of that.

The SendReadVisited function is called when a child browser process is created in order to sync visited sites, it is required because gaia notifies gecko of new visited sites through a mozContentEvent to the parent process.

To be able to quantify the cost of the call I did some profiling when opening browser with different scenarios:

· Browser with no visited sites / history:            8.5M -- 1650ms
· Browser with visited sites / history:              16.1M -- 2182ms

· Browser with patch and no visited sites / history:    9M -- 2000ms
· Browser with patch and visited sites / history:    16.9M -- 2200ms

You have the memory used by browser app after open it and the cpu time required (it doesn't mean the app is blocked).

> - What is the persistence story here? I may miss something, but it looks
> like we're not persisting anything to disk, so we start with an empty
> history after each reboot?

Yes, you are right, there is no persistence mechanism in gecko, all is managed by gaia, so after each reboot we have an empty history. Gaia persists visited sites and history in indexedDB and when System inits top sites it also takes care of notifying gecko about history sites.

> 
> In general, having comments in the code would help a lot!

Sure, I'll do.

Comment 19

[Albert [:albert]]

· Browser with no visited sites / history:            8.5M -- 1650ms
http://people.mozilla.org/~bgirard/cleopatra/#report=0a02f664b18103a06736ff45da32914de6b4b344

· Browser with visited sites / history:              16.1M -- 2182ms
http://people.mozilla.org/~bgirard/cleopatra/#report=915146aa513edca2b0a70d9650c756b2d04c99dc


· Browser with patch and no visited sites / history:    9M -- 2000ms
http://people.mozilla.org/~bgirard/cleopatra/#report=d2e5885f8e32dd1ebf58b5939393b91f6bc1ea34

· Browser with patch and visited sites / history:    16.9M -- 2200ms
http://people.mozilla.org/~bgirard/cleopatra/#report=8ea729102dabdbbaf8e578ecf4688fcb7bc2193c

Comment 20

[Albert [:albert]]

What do you think about comment 18?

Comment 21

[[:fabrice] Fabrice Desré]

The "no visited" case slowdown is a bit surprising. Do you know why the overhead would be greater there than with visited sites? I expected the opposite.

Comment 22

[Albert [:albert]]

(In reply to Fabrice Desré [:fabrice] from comment #21)
> The "no visited" case slowdown is a bit surprising. Do you know why the
> overhead would be greater there than with visited sites? I expected the
> opposite.

In order to get more accurate metrics I ran test-perf for system app. Previously I only ran profiling for browser app, but it doesn't measure the time needed for the first sync of the gaia datastore with the gecko history service.

Results for system (boot time) are not good:

- empty datastore          - no  patch  ---> 28555.2 ms
- 1000 places in datastore - no  patch  ---> 29380.6 ms

- empty datastore          - with patch ---> 30607.6 ms
- 1000 places in datastore - with patch ---> 35160.4 ms

Given results show 2 problems with the current approach:

a) When datastore is empty there are system spends 2 seconds more to load, it is not a problem with the gaia patch because I measured added code and it takes 300ms. I need to investigate the gecko patch to see where is the bottleneck.

b) The time needed to sync the gaia's places datastore with gecko history service is very high, 5 seconds. After some debugging I see that gaia - gecko message takes 2ms, the problem is in nsB2GHistory, where there are 20ms when dispatching to main thread and 130ms for the IPC communication used to notify child processes.

The patch implements a globalhistory in gecko. When device boots, gaia (system app) opens the places datastore and sends a mozContentEvent for each URI, then gecko (shell.js) receive the event and pass the URI to the implemented globalhistory service. The service is implemented as a singleton and uses the mainthread to process the incoming URI, as events are handled by parent process, main thread should notify child processes through IPC.

On the other hand, when first sync is done and new child processes are created (when browser app is started), child request the synced data to its parent browser links doesn't need to ask gaia in order to know if it is visited. Only when the user clicks a link, gaia uses the same flow of the initial synchronization, just for one URI.

Could you give me some advice to optimize the actual patch? Maybe another approach to avoid using IPC?

Comment 23

[[:fabrice] Fabrice Desré]

Did you try sending the URI in batches instead of one by one?

Comment 24

[Albert [:albert]]

(In reply to Fabrice Desré [:fabrice] from comment #23)
> Did you try sending the URI in batches instead of one by one?

Yes, I tried but it is not possible. As nsB2GHistory implements a IHistory and is accessible as a service I can't extend it with new methods allowing to send multiple URIs. Note that IHistory and nsIGlobalHistory2 only expose the methods:
 NotifyVisited(nsIURI *aURI)
 AddURI(nsIURI *aURI, bool aRedirect, bool aToplevel, nsIURI *aReferrer)

After more debugging the gaia patch I found another bottleneck, currently it syncs the datastore in order to send a message to gecko. So I removed the datastore sync and added a for loop to send the message and time spent is reduced by 3/4 seconds.

Comment 25

[ChengLin.Wu]

Dears,

Which version the attachment patch based on， 2.0 or 2.2? Please provide the information, Thanks!

Comment 26

[Albert [:albert]]

(In reply to ChengLin.Wu from comment #25)
> Dears,
> 
> Which version the attachment patch based on， 2.0 or 2.2? Please provide the
> information, Thanks!

Master (2.2)

Comment 27

[[:fabrice] Fabrice Desré]

(In reply to Albert [:albert] from comment #24)
> (In reply to Fabrice Desré [:fabrice] from comment #23)
> > Did you try sending the URI in batches instead of one by one?
> 
> Yes, I tried but it is not possible. As nsB2GHistory implements a IHistory
> and is accessible as a service I can't extend it with new methods allowing
> to send multiple URIs. Note that IHistory and nsIGlobalHistory2 only expose
> the methods:
>  NotifyVisited(nsIURI *aURI)
>  AddURI(nsIURI *aURI, bool aRedirect, bool aToplevel, nsIURI *aReferrer)

I don't understand why you can't modify these interfaces to add a AddURIs() method. Can you elaborate?

> After more debugging the gaia patch I found another bottleneck, currently it
> syncs the datastore in order to send a message to gecko. So I removed the
> datastore sync and added a for loop to send the message and time spent is
> reduced by 3/4 seconds.

Right... can you check with gaia folks if that's also something we can do?

Comment 28

[Andrea Marchesini [:baku]]

When is this gaia code called?
What about if you use: datastore-update-<name_of_the_datastore> system message?
Then you do sync only when needed.

Comment 29

[Albert [:albert]]

(In reply to Andrea Marchesini (:baku) from comment #28)
> When is this gaia code called?
> What about if you use: datastore-update-<name_of_the_datastore> system
> message?
> Then you do sync only when needed.

When a link is opened gaia sends a message to gecko with the URL. However, when the device reboots I need to populate the whole datastore to gecko. Do you mean to send datastore-update-<name_of_the_datastore> message from gaia in order to notify gecko to read from indexeddb?

Comment 30

[Maria Angeles Oteo (:oteo)]

setting ni to Andrea according to comment 28

Comment 31

[Andrea Marchesini [:baku]]

I see the problem, but instead reading from gecko the content of a DataStore (where the scheme can change, and so on), why we cannot have an API just for storing this visited URIs?

I think a simple API like (certified app, with permissions):

partial interface Navigator {
  void mozAddVisitedURL(USVString aString);
}.

Should be enough. On Gecko, these URL are stored in some sqlite db as it happens for android and browser. Reading from a dataStore, shared by gecko and gaia, seems a bit fragile.

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why not use the same mechanism that we do in desktop? I.e. have gecko store in the places database (is it still called that?) the list of visited URLs, and then use the contents of that database to color links?

Comment 33

[[:fabrice] Fabrice Desré]

(In reply to Jonas Sicking (:sicking) from comment #32)
> Why not use the same mechanism that we do in desktop? I.e. have gecko store
> in the places database (is it still called that?) the list of visited URLs,
> and then use the contents of that database to color links?

Since gaia also needs its own "places" db to provide rocketbar results, we would end up with 2 databases with mostly the same content. The idea was to avoid that, but maybe that's too hard to do while keeping an adequate performance.

The desktop places come with a lot of bells and whistles we don't need, and iirc the cost was high on device as far as memory usage was concerned.

Comment 34

[Albert [:albert]]

Attachment: Gecko patch

The patch moves the sync process from gaia to gecko. Now performance is better:

No patch           : 23260.0ms
Patch 0     records: 23242.6ms
Patch 1000  records: 23554.8ms
Patch 10000 records: 23828.4ms

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hrm... The places database on my machine is 62MB. That's a lot of data to duplicate. But i'm definitely worried about the performance and stability of using the DataStore database here.

I wonder if we can get the places database down to something like 10MB without sacrificing too much history. I'm not sure if places.sqlite contains a bunch of other information for example.

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sadly, dropping all tables except moz_places in my places.sqlite only made the size drop to 41MB. So seems like it will be hard to significantly reduce the size without sacrificing how much history we keep.

Comment 37

[Andrea Marchesini [:baku]]

Comment on attachment 8550261 [details] [diff] [review]
Gecko patch

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

Looks good!

::: b2g/components/nsB2GHistory.cpp
@@ +25,5 @@
> +
> +using namespace mozilla;
> +using mozilla::dom::Link;
> +using mozilla::dom::ContentParent;
> +using mozilla::dom::ContentChild;

if you use the namespace mozilla::dom you don't need these 3.

@@ +43,5 @@
> +ChildProcess()
> +{
> +  if (IsChildProcess()) {
> +    ContentChild* cpc = ContentChild::GetSingleton();
> +    if (!cpc)

{}

@@ +65,5 @@
> +  public:
> +    NS_DECL_ISUPPORTS
> +    NS_DECL_NSIDOMEVENTLISTENER
> +
> +    HistoryDatastoreCallback(nsB2GHistory* aHistory)

explicit

@@ +93,5 @@
> +          return;
> +        }
> +        MOZ_ASSERT(store);
> +
> +        AutoSafeJSContext cx;

Use AutoJSAPI

@@ +120,5 @@
> +HistoryDatastoreCallback::HandleEvent(nsIDOMEvent* aEvent)
> +{
> +  mRequest->RemoveEventListener(NS_LITERAL_STRING("success"), this, false);
> +
> +  AutoSafeJSContext cx;

AutoJSAPI

@@ +192,5 @@
> +    // Get the visted from the parent process
> +    InfallibleTArray<nsString> visited;
> +    ChildProcess()->SendReadVisited(&visited);
> +
> +    for (unsigned int i = 0; i < visited.Length(); i++)

{}

@@ +206,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +  }
> +
> +  return NS_OK; 

extra space.

@@ +216,5 @@
> +
> +NS_IMETHODIMP
> +nsB2GHistory::RegisterVisitedCallback(nsIURI *aURI, Link *aContent)
> +{
> +  if (!aContent || !aURI)

{}

@@ +354,5 @@
> +nsB2GHistory::Observe(nsISupports* aSubject, const char* aTopic, const char16_t* aData)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (!strcmp(aTopic, WEBAPPS_REGISTRY_READY_TOPIC)) {

if (strcmp(aTopic, ...)) {
  MOZ_ASSERT(false, ...
}

@@ +364,5 @@
> +
> +    nsAutoString name;
> +    name.AssignLiteral(DATASTORE);
> +
> +    nsresult rv = service->IsDataStoreReady(name, manifestURL, this);

Just do:

nsresult rv = service->IsDataStoreRead(NS_LITERAL_STRING(DATASTORE), NS_LITERAL_STRING(MANIFEST_URL), this)

::: b2g/components/nsB2GHistory.h
@@ +2,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef NS_B2GHISTORY_H

lowercase

@@ +11,5 @@
> +#include "nsTPriorityQueue.h"
> +#include "nsIRunnable.h"
> +#include "nsIURI.h"
> +#include "nsIObserver.h"
> +#include "nsIDataStoreService.h"

alphabetic order

@@ +14,5 @@
> +#include "nsIObserver.h"
> +#include "nsIDataStoreService.h"
> +
> +#define NS_B2GHISTORY_CID \
> +{ 0x33a392b6, 0x5932, 0x11e4, { 0xb8, 0x92, 0x73, 0xa3, 0x27, 0xc7, 0xa9, 0xb4 } }

is it 80chars?

@@ +36,5 @@
> +   */
> +  static nsB2GHistory* GetSingleton();
> +  nsresult Init();
> +
> +  nsB2GHistory();

private ctor.

@@ +38,5 @@
> +  nsresult Init();
> +
> +  nsB2GHistory();
> +
> +  void getVisited(nsTArray<nsString>* aVisited);

GetVisited

@@ +39,5 @@
> +
> +  nsB2GHistory();
> +
> +  void getVisited(nsTArray<nsString>* aVisited);
> +  nsresult addVisited(const nsString& uri);

AddVisited + nsAString

@@ +40,5 @@
> +  nsB2GHistory();
> +
> +  void getVisited(nsTArray<nsString>* aVisited);
> +  nsresult addVisited(const nsString& uri);
> +  nsresult clearVisited();

ClearVisited

@@ +45,5 @@
> +
> +private:
> +  ~nsB2GHistory() {}
> +
> +  static nsB2GHistory* sHistory;

Remove it from here and put it in an anonymous namespace in the cpp file.

@@ +47,5 @@
> +  ~nsB2GHistory() {}
> +
> +  static nsB2GHistory* sHistory;
> +
> +  nsDataHashtable<nsStringHashKey, nsTArray<mozilla::dom::Link *> *> mListeners;

space before * is not needed anymore.

::: b2g/components/nsB2GProxyHistory.cpp
@@ +1,1 @@
> +#include "nsB2GProxyHistory.h"

No initial comments?

@@ +17,5 @@
> +  /* destructor code */
> +}
> +
> +/* void addURI (in nsIURI aURI, in boolean aRedirect, in boolean aToplevel, in nsIURI aReferrer); */
> +NS_IMETHODIMP nsB2GProxyHistory::AddURI(nsIURI *aURI, bool aRedirect, bool aToplevel, nsIURI *aReferrer)

new line after NS_IMETHODIMP

@@ +32,5 @@
> +
> +/* boolean isVisited (in nsIURI aURI); */
> +NS_IMETHODIMP nsB2GProxyHistory::IsVisited(nsIURI *aURI, bool *_retval)
> +{
> +    return NS_ERROR_NOT_IMPLEMENTED;

2 spaces indentation.

::: dom/datastore/DataStoreService.cpp
@@ +934,5 @@
> +                                   const nsAString& aManifestURL,
> +                                   nsIDataStoreReadyCallback* aCallback)
> +{
> +  ASSERT_PARENT_PROCESS()
> +  MOZ_ASSERT(NS_IsMainThread());

MOZ_ASSERT(aCallback)

@@ +943,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCOMPtr<mozIApplication> app;
> +  appsService->GetAppByManifestURL(aManifestURL, getter_AddRefs(app));

if (!app)

@@ +953,5 @@
> +  }
> +
> +  HashApp* apps = nullptr;
> +  if (!mStores.Get(aName, &apps)) {
> +    return NS_ERROR_FAILURE;

would be nice to have a better error message.

@@ +961,5 @@
> +  if (!apps->Get(appId, &info)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  if (!info->mEnabled) {

Can you store this array into the DataStoreInfo? In this case you don't have to allocate it and you can do an assertion like:

MOZ_ASSERT(!info->mEnabled || info->mEnableListeners.IsEmpty());

@@ +972,5 @@
> +
> +    return NS_OK;
> +  }
> +
> +  aCallback->DataStoreAvailable();

Just to keep the same behavior, use a runnable to dispatch this DataStoreAvailable callback.

::: dom/datastore/nsIDataStoreService.idl
@@ +14,5 @@
> +{
> +  /**
> +   * Callback function used to report that datastore is enabled.
> +   */
> +  void dataStoreAvailable();

I think it would be nice if this callback receives DOMString name and DOMString manifest.
In case the caller wants to listen for more than 1 datastore.

::: dom/ipc/PContent.ipdl
@@ +715,5 @@
>      // nsIPermissionManager messages
>      sync ReadPermissions() returns (Permission[] permissions);
>  
> +    sync ReadVisited() returns (nsString[] visited);
> +    

extra space.

Comment 38

[Albert [:albert]]

Attachment: Gecko patch

Changes from comment 37

Comment 39

[Andrea Marchesini [:baku]]

Comment on attachment 8551392 [details] [diff] [review]
Gecko patch

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

I have a big concern: we have 1 list of visited urls, shared between apps. Can this be a problem?
I guess we should have a list of visited urls PER app otherwise an malicious app can know if the user has visited something... is it a real problem?

I would like to see this patch again with these comments fixed. Thanks!

::: b2g/chrome/content/shell.js
@@ +736,5 @@
>          break;
> +      case 'link-visited':
> +        {
> +          let history = Cc["@mozilla.org/browser/proxyhistory;1"]
> +                            .getService(Ci.nsIGlobalHistory2);

too much indentation

@@ +738,5 @@
> +        {
> +          let history = Cc["@mozilla.org/browser/proxyhistory;1"]
> +                            .getService(Ci.nsIGlobalHistory2);
> +          let IOService = Cc["@mozilla.org/network/io-service;1"]
> +                          .getService(Ci.nsIIOService);

too less indentation

@@ +746,5 @@
> +          break;
> +        }
> +      case 'clear-history':
> +          let history = Cc["@mozilla.org/browser/proxyhistory;1"]
> +                            .getService(Ci.nsIGlobalHistory2);

Too much here too.

::: b2g/components/nsB2GHistory.cpp
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsThreadUtils.h"
> +#include "nsB2GHistory.h"

alphabetic order. And nsB2GHistory on top.

@@ +97,5 @@
> +        }
> +        MOZ_ASSERT(store);
> +
> +        AutoJSAPI jsapi;
> +        jsapi.Init();

You cannot do this. Read the documentation of AUtoJSAPI.

@@ +100,5 @@
> +        AutoJSAPI jsapi;
> +        jsapi.Init();
> +        JSContext* cx = jsapi.cx();
> +
> +        ErrorResult errorr;

error

@@ +110,5 @@
> +        }
> +
> +        mRequest->EventTarget::AddEventListener(NS_LITERAL_STRING("success"), this, false);
> +        return;
> +      }

Just to be use, add a MOZ_CRASH("aStatus cannot be something different than Success and Error");

@@ +127,5 @@
> +{
> +  mRequest->RemoveEventListener(NS_LITERAL_STRING("success"), this, false);
> +
> +  AutoJSAPI jsapi;
> +  jsapi.Init();

same here.

@@ +140,5 @@
> +    return NS_OK;
> +  }
> +
> +  // Parse result
> +  if (result.isObject()) {

What about if it's not an object? I guess you should assert it.

@@ +142,5 @@
> +
> +  // Parse result
> +  if (result.isObject()) {
> +    JS::Rooted<JSObject*> resultObj(cx, &result.toObject());
> +    if (JS_IsArrayObject(cx, resultObj)) {

same here.

@@ +201,5 @@
> +    // Get the visted from the parent process
> +    InfallibleTArray<nsString> visited;
> +    ChildProcess()->SendReadVisited(&visited);
> +
> +    for (unsigned int i = 0; i < visited.Length(); i++) {

uint32_t

@@ +219,5 @@
> +
> +  return NS_OK;
> +}
> +
> +nsB2GHistory::nsB2GHistory()

Move this at the beginning of the file.

@@ +227,5 @@
> +NS_IMETHODIMP
> +nsB2GHistory::RegisterVisitedCallback(nsIURI *aURI, Link *aContent)
> +{
> +  if (!aContent || !aURI) {
> +    return NS_OK;

Why NS_OK? Maybe NS_ERROR_FAILURE? Or better:
MOZ_ASSERT(aURI && aContent);

@@ +232,5 @@
> +  }
> +
> +  nsAutoCString uri;
> +  nsresult rv = aURI->GetSpec(uri);
> +  if (NS_FAILED(rv)) return rv;

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +242,5 @@
> +    return NS_OK;
> +  }
> +
> +  nsTArray<Link*>* list = mListeners.Get(uriString);
> +  if (! list) {

if (!list)

@@ +253,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsB2GHistory::UnregisterVisitedCallback(nsIURI *aURI, Link *aContent)
> +{

MOZ_ASSERT(aURI && aContent);

@@ +263,5 @@
> +  if (NS_FAILED(rv)) return rv;
> +  NS_ConvertUTF8toUTF16 uriString(uri);
> +
> +  nsTArray<Link*>* list = mListeners.Get(uriString);
> +  if (! list)

if (!list) {

@@ +277,5 @@
> +}
> +
> +void
> +nsB2GHistory::GetVisited(nsTArray<nsString>* aVisited)
> +{

MOZ_ASSERT(aVisited);

@@ +283,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsB2GHistory::VisitURI(nsIURI *aURI, nsIURI *aLastVisitedURI, uint32_t aFlags)
> +{

MOZ_ASSERT(aURI && aLastVisitedURI);

@@ +289,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsB2GHistory::SetURITitle(nsIURI *aURI, const nsAString& aTitle)
> +{

MOZ_ASSERT(aURI);

Add a comment saying that this method is not actually implemented.

@@ +296,5 @@
> +
> +NS_IMETHODIMP
> +nsB2GHistory::NotifyVisited(nsIURI *aURI)
> +{
> +  if (!gB2GHistory)

{}

@@ +301,5 @@
> +    return NS_ERROR_FAILURE;
> +
> +  if (aURI) {
> +    nsAutoCString utf8Spec;
> +    (void)aURI->GetSpec(utf8Spec);

remove this (void) and do nsresult rv = ... if (NS_WARN_IF(NS_FAILED(rv)) .. return rv;

@@ +307,5 @@
> +    return AddVisited(NS_ConvertUTF8toUTF16(utf8Spec));
> +  }
> +
> +  // If no URI, delete message
> +  ClearVisited();

return ClearVisited();

@@ +313,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +nsB2GHistory::AddVisited(const nsString& uri)

aURI

@@ +341,5 @@
> +
> +  return NS_OK;
> +}
> +
> +nsresult nsB2GHistory::ClearVisited()

nsresult in a new line.

@@ +392,5 @@
> +
> +  nsRefPtr<HistoryDatastoreCallback> callback = new HistoryDatastoreCallback(gB2GHistory);
> +
> +  Sequence<nsString> dbs;
> +  dbs.AppendElement(NS_LITERAL_STRING(DATASTOREDB_REVISION));

It doens't seem right that you want the revision db. Maybe you want 'data'.

::: b2g/components/nsB2GHistory.h
@@ +42,5 @@
> +  nsresult ClearVisited();
> +
> +private:
> +  nsB2GHistory();
> +  ~nsB2GHistory() {}

Move it to the cpp file and does:

~nsB2GHistory()
{
  MOZ_ASSERT(mListeners.Count() == 0);
}

@@ +44,5 @@
> +private:
> +  nsB2GHistory();
> +  ~nsB2GHistory() {}
> +
> +  nsDataHashtable<nsStringHashKey, nsTArray<mozilla::dom::Link*>*> mListeners;

Can you write a comment saying that the List objects will be unregistered before going away?

::: b2g/components/nsB2GProxyHistory.cpp
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsB2GProxyHistory.h"
> +#include "mozilla/Services.h"
> +#include "IHistory.h"

alphabetic order.

@@ +12,5 @@
> +NS_IMPL_ISUPPORTS(nsB2GProxyHistory, nsIGlobalHistory2)
> +
> +nsB2GProxyHistory::nsB2GProxyHistory()
> +{
> +  /* member initializers and constructor code */

remove this comment...

@@ +17,5 @@
> +}
> +
> +nsB2GProxyHistory::~nsB2GProxyHistory()
> +{
> +  /* destructor code */

here too.

::: b2g/components/nsB2GProxyHistory.h
@@ +13,5 @@
> +
> +#define NS_B2GPROXYHISTORY_CONTRACTID \
> +"@mozilla.org/browser/proxyhistory;1"
> +
> +class nsB2GProxyHistory : public nsIGlobalHistory2

MOZ_FINAL ?

@@ +24,5 @@
> +
> +private:
> +  ~nsB2GProxyHistory();
> +
> +protected:

remove this.

::: dom/datastore/DataStoreService.cpp
@@ +115,5 @@
>  
>    // A DataStore is enabled when it has its first revision.
>    bool mEnabled;
> +
> +  nsTArray<nsIDataStoreReadyCallback*> mEnableListeners;

probably you want to keep these objects alive. nsTArray<nsCOMPtr<...

@@ +801,5 @@
>    ASSERT_PARENT_PROCESS()
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    HashApp* apps = nullptr;
> +

why this line ? :)

@@ +859,5 @@
>                                  const nsAString& aName,
>                                  const nsAString& aOwner,
>                                  nsISupports** aDataStores)
>  {
> +

or this?

@@ +931,5 @@
>    return NS_OK;
>  }
>  
> +
> +

not too many lines here.

@@ +932,5 @@
>  }
>  
> +
> +
> +class DataStoreReadyDispatcher : public nsRunnable

MOZ_FINAL

@@ +942,5 @@
> +    : mCallback(aCallback),
> +      mName(aName),
> +      mManifestURL(aManifestURL)
> +  { }
> +

MOZ_ASSERT(aCallback)

@@ +943,5 @@
> +      mName(aName),
> +      mManifestURL(aManifestURL)
> +  { }
> +
> +  NS_IMETHOD Run() {

{ in a new line.

@@ +949,5 @@
> +    return NS_OK;
> +  }
> +
> +private:
> +  nsIDataStoreReadyCallback* mCallback;

Keep this object alive using nsCOMPtr

@@ +972,5 @@
> +  }
> +
> +  nsCOMPtr<mozIApplication> app;
> +  appsService->GetAppByManifestURL(aManifestURL, getter_AddRefs(app));
> +  if (!app) {

NS_WARN_IF + check the return value of GetAppManifetURL

@@ +978,5 @@
> +  }
> +
> +  uint32_t appId;
> +  nsresult rv = app->GetLocalId(&appId);
> +  if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +989,5 @@
> +  }
> +
> +  DataStoreInfo* info = nullptr;
> +  if (!apps->Get(appId, &info)) {
> +    return NS_ERROR_FAILURE;

I would say: return NS_ERROR_DOM_NOT_FOUND_ERR here too.

@@ +999,5 @@
> +  }
> +
> +  nsCOMPtr<nsIRunnable> r = new DataStoreReadyDispatcher(aCallback, aName,
> +                                                         aManifestURL);
> +  NS_DispatchToMainThread(r);

NS_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(r)));

@@ +1172,5 @@
>      return NS_ERROR_OUT_OF_MEMORY;
>    }
>  
>    HashApp* apps = nullptr;
> +

no extra line.

@@ +1362,5 @@
>      if (mStores.Get(aName, &apps) && apps->Get(aAppId, &info)) {
>        info->Enable();
> +
> +      // Notify datastore available
> +      for (unsigned int i = 0; i < info->mEnableListeners.Length(); i++) {

uint32_t

::: dom/datastore/nsIDataStoreService.idl
@@ +41,5 @@
>    nsIArray getAppManifestURLsForDataStore(in DOMString name);
>  
>    boolean checkPermission(in nsIPrincipal principal);
> +
> +  [noscript]

change the UUID of the nsIDataStoreService.

::: dom/ipc/ContentChild.cpp
@@ +1993,5 @@
> +    nsCOMPtr<IHistory> history = services::GetHistoryService();
> +    nsB2GHistory* b2gHistory = static_cast<nsB2GHistory*>(history.get());
> +    NS_ABORT_IF_FALSE(b2gHistory,
> +                 "We have no b2gHistory in the Chrome process !");
> +

indentation

@@ +2010,5 @@
> +#if MOZ_B2G_HISTORY
> +    nsCOMPtr<IHistory> history = services::GetHistoryService();
> +    nsB2GHistory* b2gHistory = static_cast<nsB2GHistory*>(history.get());
> +    NS_ABORT_IF_FALSE(b2gHistory,
> +                 "We have no b2gHistory in the Chrome process !");

indentation

::: dom/ipc/ContentParent.cpp
@@ +2428,5 @@
> +#ifdef MOZ_B2G_HISTORY
> +    nsCOMPtr<IHistory> history = services::GetHistoryService();
> +    nsB2GHistory* b2gHistory = static_cast<nsB2GHistory*>(history.get());
> +    NS_ABORT_IF_FALSE(b2gHistory,
> +                 "We have no b2gHistory in the Chrome process !");

indentation

Comment 40

[Albert [:albert]]

(In reply to Andrea Marchesini (:baku) from comment #39)
> Comment on attachment 8551392 [details] [diff] [review]
> Gecko patch
> 
> Review of attachment 8551392 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have a big concern: we have 1 list of visited urls, shared between apps.
> Can this be a problem?
> I guess we should have a list of visited urls PER app otherwise an malicious
> app can know if the user has visited something... is it a real problem?

The list of visited urls is not exposed to apps but clear and addvisited through 'link-visited' message.

> @@ +97,5 @@
> > +        }
> > +        MOZ_ASSERT(store);
> > +
> > +        AutoJSAPI jsapi;
> > +        jsapi.Init();
> 
> You cannot do this. Read the documentation of AUtoJSAPI.

I didn't find documentation of AutoJSAPI, where is it?

I saw that AudioContext and Geolocation are calling init like me:
https://mxr.mozilla.org/mozilla-central/source/dom/geolocation/nsGeolocationSettings.cpp#235
https://mxr.mozilla.org/mozilla-central/source/dom/media/webaudio/AudioContext.cpp#458

Comment 41

[Andrea Marchesini [:baku]]

> I didn't find documentation of AutoJSAPI, where is it?

in ./dom/base/ScriptSettings.h it says:

  // This uses the SafeJSContext (or worker equivalent), and enters a null
  // compartment, so that the consumer is forced to select a compartment to
  // enter before manipulating objects.
  void Init();

> I saw that AudioContext and Geolocation are calling init like me:
> https://mxr.mozilla.org/mozilla-central/source/dom/geolocation/
> nsGeolocationSettings.cpp#235
> https://mxr.mozilla.org/mozilla-central/source/dom/media/webaudio/
> AudioContext.cpp#458

Right, but then AudioContext does:
  JSAutoCompartment ac(cx, aBuffer.Obj());

I'll ask bholley about nsGeolocationSettings.cpp.

Comment 42

[Albert [:albert]]

Attachment: Gecko patch

Changes from comment 39

As talked in IRC I use AutoSafeJSContext until we clarify if it is ok.

Comment 43

[Andrea Marchesini [:baku]]

Comment on attachment 8551750 [details] [diff] [review]
Gecko patch

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

Looks good to me. Maybe a nice follow up is to limit the number of visited URLs to keep in memory.

::: b2g/chrome/content/shell.js
@@ +735,5 @@
>          DoCommandHelper.handleEvent(detail.cmd);
>          break;
> +      case 'link-visited':
> +        {
> +          let history = Cc["@mozilla.org/browser/proxyhistory;1"]

move this when you actually use history.

::: b2g/components/nsB2GHistory.cpp
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

add 
/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
/* vim: set ts=8 sts=2 et sw=2 tw=80: */

at the top of the file.

@@ +62,5 @@
> +
> +class HistoryDatastoreCallback MOZ_FINAL : public DataStoreDBCallback,
> +                                           public nsIDOMEventListener
> +{
> +  public:

We usually indent like this:

class HistoryDatastoreCallback MOZ_FINAL ...
{
public:
  NS_DECL_ISUPPORTS
  ...
};

@@ +82,5 @@
> +      if (aStatus == Error) {
> +        NS_WARNING("Failed to open database.");
> +        return;
> +      }
> +

MOZ_ASSERT(aStatus == Success, "aStatus cannot be something different than Success and Error");

and remove if (aStatus == Success) ..

@@ +112,5 @@
> +
> +  private:
> +    ~HistoryDatastoreCallback() {}
> +    nsRefPtr<indexedDB::IDBRequest> mRequest;
> +    nsB2GHistory* mHistory;

what does keep this object alive? Maybe you should do: nsRefPtr<nsB2GHistory> ... ?
Or better, if this is a singleton, you don't need to keep this variable member. When you need to use it, just do:

nsB2GHistory::GetSingleton();

@@ +134,5 @@
> +  }
> +
> +  // Parse result
> +  MOZ_ASSERT(result.isObject());
> +

Write a comment about the kind of object you expect to find here.

@@ +162,5 @@
> +        nsAutoJSString str;
> +        bool ok = str.init(cx, jsstr);
> +        NS_ENSURE_TRUE(ok, NS_ERROR_FAILURE);
> +
> +        mHistory->AddVisited(str);

here you can do:

nsRefPtr<nsB2GHistory> history = nsB2GHistory::GetSingleton();
MOZ_ASSERT(history); // these 2 lines before the for(...)

history->AddVisited(str);

@@ +190,5 @@
> +
> +NS_IMPL_ISUPPORTS(nsB2GHistory, IHistory, nsIObserver, nsIDataStoreReadyCallback)
> +
> +nsB2GHistory::nsB2GHistory()
> +{

Nothing deletes the gB2GHistory object. Probably you want to do is: ClearOnShutdown(&gB2GHistory);

@@ +263,5 @@
> +  if (NS_FAILED(rv)) return rv;
> +  NS_ConvertUTF8toUTF16 uriString(uri);
> +
> +  nsTArray<Link*>* list = mListeners.Get(uriString);
> +  if (!list)

if (!list) {
  return NS_OK;
}

@@ +285,5 @@
> +
> +NS_IMETHODIMP
> +nsB2GHistory::VisitURI(nsIURI *aURI, nsIURI *aLastVisitedURI, uint32_t aFlags)
> +{
> +  MOZ_ASSERT(aURI && aLastVisitedURI);

doing nothing, this assert can be removed.

@@ +301,5 @@
> +NS_IMETHODIMP
> +nsB2GHistory::NotifyVisited(nsIURI *aURI)
> +{
> +  if (!gB2GHistory) {
> +    return NS_ERROR_FAILURE;

how can it be?

@@ +321,5 @@
> +
> +nsresult
> +nsB2GHistory::AddVisited(const nsString& aUri)
> +{
> +  gB2GHistory->mVisitedURIs.PutEntry(aUri);

gB2GHistory is 'this', right? So you can just do:
mVisitedURI.... ?

@@ +323,5 @@
> +nsB2GHistory::AddVisited(const nsString& aUri)
> +{
> +  gB2GHistory->mVisitedURIs.PutEntry(aUri);
> +
> +  if (!IsChildProcess()) {

if (IsChildProcess()) { 
  ...
} else {
}

instead if (!IsChildProcess()).

@@ +332,5 @@
> +        return NS_ERROR_FAILURE;
> +      }
> +    }
> +  } else {
> +    nsTArray<Link*>* list = gB2GHistory->mListeners.Get(aUri);

mListeners.Get(..)

@@ +351,5 @@
> +nsresult
> +nsB2GHistory::ClearVisited()
> +{
> +  gB2GHistory->mVisitedURIs.Clear();
> +

same here: gB2GHistory is this object.

@@ +376,5 @@
> +  if (strcmp(aTopic, WEBAPPS_REGISTRY_READY_TOPIC)) {
> +    MOZ_ASSERT(false, "B2GHistory got unexpected topic!");
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +

At this point, you can remove the observer.

@@ +381,5 @@
> +  nsRefPtr<DataStoreService> service = DataStoreService::GetOrCreate();
> +  NS_ENSURE_TRUE(service, NS_ERROR_FAILURE);
> +
> +  nsresult rv = service->IsDataStoreReady(NS_LITERAL_STRING(DATASTORE),
> +                                          NS_LITERAL_STRING(MANIFEST_URL), this);

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +392,5 @@
> +
> +nsresult
> +nsB2GHistory::DataStoreAvailable(const nsAString& aName,
> +                                 const nsAString& aManifestURL)
> +{

Just to be 100% sure I would add:  MOZ_ASSERT(aName.EqualLiteral(DATASTORE) && aManifestURL.EqualsLiteral(MANIFEST_URL));

@@ +396,5 @@
> +{
> +  nsRefPtr<DataStoreDB> db = new DataStoreDB(NS_LITERAL_STRING(MANIFEST_URL),
> +                                             NS_LITERAL_STRING(DATASTORE));
> +
> +  nsRefPtr<HistoryDatastoreCallback> callback = new HistoryDatastoreCallback(gB2GHistory);

You don't need to pass gB2GHistory here.

::: b2g/components/nsB2GHistory.h
@@ +37,5 @@
> +  static already_AddRefed<nsB2GHistory> GetSingleton();
> +  nsresult Init();
> +
> +  void GetVisited(nsTArray<nsString>* aVisited);
> +  nsresult AddVisited(const nsString& uri);

I really think this can be const nsAString&

::: b2g/components/nsB2GProxyHistory.cpp
@@ +6,5 @@
> +#include "IHistory.h"
> +#include "mozilla/Services.h"
> +#include "nsCOMPtr.h"
> +
> +using namespace mozilla;

why do you need this? For services? If just for that, do mozilla::services::..

@@ +45,5 @@
> + nsB2GProxyHistory::SetPageTitle(nsIURI *aURI, const nsAString & aTitle)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +

extra line.

::: dom/datastore/DataStoreService.cpp
@@ +988,5 @@
> +  }
> +
> +  DataStoreInfo* info = nullptr;
> +  if (!apps->Get(appId, &info)) {
> +    return NS_ERROR_FAILURE;

I guess also here we want to return DOM_NOT_FOND_ERR.

::: dom/datastore/nsIDataStoreService.idl
@@ +40,5 @@
>  
>    nsIArray getAppManifestURLsForDataStore(in DOMString name);
>  
>    boolean checkPermission(in nsIPrincipal principal);
> +

change the UUID of nsIDataStoreService.

Comment 44

[Albert [:albert]]

Attachment: Gecko patch

Changes from comment 43

Comment 45

[Albert [:albert]]

Comment on attachment 8510096 [details] [review]
Gaia patch

The patch adds messages from gaia to gecko (mozContentEvent) to notify history events (new visited URL and clear history).

Comment 46

[Dale Harvey (:daleharvey)]

Comment on attachment 8510096 [details] [review]
Gaia patch

This looks awesome, thanks

Comment 47

[[:fabrice] Fabrice Desré]

Comment on attachment 8551891 [details] [diff] [review]
Gecko patch

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

Sorry for the terrible review time here.
I don't like that this design ties the gecko side to gaia implementation details of the datastore, including the structure of what is stored in indexedDB. That will not work at all with alternative UIs that reuse the b2g runtime, and we will need that.

So unfortunately I guess we're back to the drawing board :(

Comment 48

[Albert [:albert]]

(In reply to Fabrice Desré [:fabrice] from comment #47)
> Comment on attachment 8551891 [details] [diff] [review]
> Gecko patch
> 
> Review of attachment 8551891 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the terrible review time here.
> I don't like that this design ties the gecko side to gaia implementation
> details of the datastore, including the structure of what is stored in
> indexedDB. That will not work at all with alternative UIs that reuse the b2g
> runtime, and we will need that.
> 
> So unfortunately I guess we're back to the drawing board :(

Then I only see a couple of alternatives:

- Use a sqlite database in gecko to store visited URLs like desktop and android do. It will not mean to duplicate the data of gaia because gaia stores more stuff, not only the url.

- Move the management of visited URLs to gecko and create a private webapi to let gaia retrieve the visited URLs.

What do you think? Do you see another alternatives?

Comment 49

[[:fabrice] Fabrice Desré]

(In reply to Albert [:albert] from comment #48)

> Then I only see a couple of alternatives:
> 
> - Use a sqlite database in gecko to store visited URLs like desktop and
> android do. It will not mean to duplicate the data of gaia because gaia
> stores more stuff, not only the url.
> 
> - Move the management of visited URLs to gecko and create a private webapi
> to let gaia retrieve the visited URLs.
> 
> What do you think? Do you see another alternatives?

I feel the first option is better. If we can have data flow only one way that's easier to manage.

Comment 50

[Kai-Chih Hu [:khu]]

Feature landing date(2/23) was passed. Minus feature-b2g:2.2 for now. If we would like to put this into 2.2, please use approval-mozilla-b2g37? and approval-gaia-v2.2?. Release management team will triage the approvals. 

Thank you.

Comment 51

[Albert [:albert]]

Attachment: Gecko patch part 1

Adds B2G History component

Comment 52

[Albert [:albert]]

Attachment: Gecko patch part 2

Adds persistence using mozStorage

Comment 53

[Andrea Marchesini [:baku]]

Comment on attachment 8571918 [details] [diff] [review]
Gecko patch part 1

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

::: b2g/chrome/content/shell.js
@@ +720,5 @@
> +        }
> +      case 'clear-history':
> +          let history = Cc["@mozilla.org/browser/proxyhistory;1"]
> +                          .getService(Ci.nsIGlobalHistory2);
> +        history.addURI(null, false, false, null);

this seems an hack. Why a null uri should clear the history?

::: b2g/components/nsB2GHistory.cpp
@@ +51,5 @@
> +
> +} /* anonymous namespace */
> +
> +/*static*/
> +already_AddRefed<nsB2GHistory>

you can return a nsB2GHistory*

@@ +64,5 @@
> +    gB2GHistory = service;
> +  }
> +
> +  nsRefPtr<nsB2GHistory> service = gB2GHistory.get();
> +  return service.forget();

return service;

@@ +130,5 @@
> +  MOZ_ASSERT(aURI && aContent);
> +
> +  nsAutoCString uri;
> +  nsresult rv = aURI->GetSpec(uri);
> +  if (NS_FAILED(rv)) return rv;

NS_WARN_IF ...

@@ +133,5 @@
> +  nsresult rv = aURI->GetSpec(uri);
> +  if (NS_FAILED(rv)) return rv;
> +  NS_ConvertUTF8toUTF16 uriString(uri);
> +
> +  nsTArray<Link*>* list = mListeners.Get(uriString);

can it happen? I mean an unregister without a register? Maybe we should assert here.

@@ +164,5 @@
> +NS_IMETHODIMP
> +nsB2GHistory::SetURITitle(nsIURI *aURI, const nsAString& aTitle)
> +{
> +  /* Not implemented */
> +  MOZ_ASSERT(aURI);

remove this assert if this is not going to be implemented.

@@ +182,5 @@
> +    return AddVisited(NS_ConvertUTF8toUTF16(utf8Spec));
> +  }
> +
> +  // If no URI, delete message
> +  return ClearVisited();

yeah... this seems a bit an hack.

@@ +224,5 @@
> +    nsTArray<ContentParent*> parents;
> +    ContentParent::GetAll(parents);
> +    for (uint32_t i = 0; i < parents.Length(); ++i) {
> +      if (!parents[i]->SendClearVisited()) {
> +        return NS_ERROR_FAILURE;

I guess you don't want to return if 1 fails. Just write a NS_WARNING message and go on.

::: configure.in
@@ +5745,5 @@
>  AC_SUBST(MOZ_CHILD_PERMISSIONS)
>  
>  dnl ========================================================
> +dnl = Enable B2G History instead of Places
> +dnl ========================================================

any particular reason why you want this feature to be configurable?

::: dom/datastore/DataStoreService.cpp
@@ +928,5 @@
>    promise.forget(aDataStores);
>    return NS_OK;
>  }
>  
> +class DataStoreReadyDispatcher MOZ_FINAL : public nsRunnable

Do we still need this?

::: dom/datastore/nsIDataStoreService.idl
@@ +10,5 @@
>  interface nsIArray;
>  
> +[function, uuid(bc3e1971-b283-4e9b-86a7-a29cdb277617)]
> +interface nsIDataStoreReadyCallback : nsISupports
> +{

I don't think we need this change.

Comment 54

[Andrea Marchesini [:baku]]

Comment on attachment 8571919 [details] [diff] [review]
Gecko patch part 2

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

Looks good. Add some NS_WARN_IF here and there, add some rv check.
But more important, would be nice if you can add some test! Maybe some gtest?

::: b2g/components/nsB2GHistory.cpp
@@ +100,5 @@
> +
> +    return NS_OK;
> +  }
> +
> +  InitDB();

This can fail, right? rv = ..

@@ +117,5 @@
> +nsB2GHistory::InitDB()
> +{
> +  nsCOMPtr<nsIFile> historyFile;
> +  nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(historyFile));
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);

if (NS_WARN_IF(NS_FAILED(rv))) { ...

@@ +120,5 @@
> +  nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(historyFile));
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);
> +
> +  rv = historyFile->AppendNative(NS_LITERAL_CSTRING(kHistoryFileName));
> +  NS_ENSURE_SUCCESS(rv, rv);

same here.

@@ +123,5 @@
> +  rv = historyFile->AppendNative(NS_LITERAL_CSTRING(kHistoryFileName));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<mozIStorageService> storage = do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID);
> +  if (!storage) {

NS_WARN_IF

@@ +141,5 @@
> +    rv = storage->OpenDatabase(historyFile, getter_AddRefs(mDBConn));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    mDBConn->GetConnectionReady(&ready);
> +    if (!ready)

1. {}
2. NS_WARN_IF

@@ +146,5 @@
> +      return NS_ERROR_UNEXPECTED;
> +  }
> +
> +  bool tableExists = false;
> +  mDBConn->TableExists(NS_LITERAL_CSTRING("visited_urls"), &tableExists);

can this fail?

@@ +153,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +
> +  // make operations on the table asynchronous, for performance
> +  mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("PRAGMA synchronous = OFF"));

can this fail?

@@ +161,5 @@
> +    "INSERT INTO visited_urls (id, url) "
> +    "VALUES (?1, ?2)"), getter_AddRefs(mStmtInsert));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (tableExists)

{}

@@ +172,5 @@
> +nsB2GHistory::CreateTable()
> +{
> +  // set the schema version, before creating the table
> +  nsresult rv = mDBConn->SetSchemaVersion(VISITED_SCHEMA_VERSION);
> +  if (NS_FAILED(rv)) return rv;

NS_WARN_IF

@@ +356,5 @@
> +nsB2GHistory::InsertURL(const nsAString& aUri)
> +{
> +  uint32_t id = nsStringHashKey::HashKey(&aUri);
> +  nsresult rv = mStmtInsert->BindInt64ByIndex(0, id);
> +  if (NS_FAILED(rv)) {

NS_WARN_IF here and below

@@ +383,5 @@
>          return NS_ERROR_FAILURE;
>        }
>      }
> +
> +    ClearDB();

rv =  ?

::: b2g/components/nsB2GHistory.h
@@ +19,2 @@
>  #define NS_B2GHISTORY_CID \
>  {0x33a392b6, 0x5932, 0x11e4, {0xb8, 0x92, 0x73, 0xa3, 0x27, 0xc7, 0xa9, 0xb4}}

change this.

Comment 55

[Albert [:albert]]

Attachment: Gecko patch part 1 v2

Changes from comment 53

Comment 56

[Albert [:albert]]

Attachment: Gecko patch part 2 v2

Changes from comment 54

Comment 57

[Albert [:albert]]

(In reply to Andrea Marchesini (:baku) from comment #53)
> Comment on attachment 8571918 [details] [diff] [review]
> Gecko patch part 1
> 
> Review of attachment 8571918 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/chrome/content/shell.js
> @@ +720,5 @@
> > +        }
> > +      case 'clear-history':
> > +          let history = Cc["@mozilla.org/browser/proxyhistory;1"]
> > +                          .getService(Ci.nsIGlobalHistory2);
> > +        history.addURI(null, false, false, null);
> 
> this seems an hack. Why a null uri should clear the history?
> 
> @@ +182,5 @@
> > +    return AddVisited(NS_ConvertUTF8toUTF16(utf8Spec));
> > +  }
> > +
> > +  // If no URI, delete message
> > +  return ClearVisited();
> 
> yeah... this seems a bit an hack.

It is because I won't add a new method to the nsIGlobalHistory2 as it is not needed for other history services. For that reason I used the null, do you prefer to add a method to nsIGlobalHistory2?

> ::: configure.in
> @@ +5745,5 @@
> >  AC_SUBST(MOZ_CHILD_PERMISSIONS)
> >  
> >  dnl ========================================================
> > +dnl = Enable B2G History instead of Places
> > +dnl ========================================================
> 
> any particular reason why you want this feature to be configurable?

No, just to be able to enable / disable it, should I remove it?

Comment 58

[Andrea Marchesini [:baku]]

> It is because I won't add a new method to the nsIGlobalHistory2 as it is not
> needed for other history services. For that reason I used the null, do you
> prefer to add a method to nsIGlobalHistory2?

What about if you create a new nsIB2GGlobalHistory interface that extends nsIGlobalHistory2.
Here you expose ClearVisisted().

> No, just to be able to enable / disable it, should I remove it?

I think so. Let's see what Fabrice says.

Comment 59

[Wilfred Mathanaraj [:WDM]]

need review from Fabrice... please

Comment 60

[Albert [:albert]]

Attachment: Gecko patch part 1 v3

Created a new nsIB2GGlobalHistory interface that extends nsIGlobalHistory2 to expose ClearVisisted()

Comment 61

[Albert [:albert]]

Attachment: Gecko patch part 2 v3

Rebase

Comment 62

[[:fabrice] Fabrice Desré]

Comment on attachment 8577355 [details] [diff] [review]
Gecko patch part 1 v3

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

::: b2g/chrome/content/shell.js
@@ +700,5 @@
> +      case 'link-visited':
> +        {
> +          let IOService = Cc["@mozilla.org/network/io-service;1"]
> +                            .getService(Ci.nsIIOService);
> +          let uri = IOService.newURI(detail.url, null, null);

just use Services.io.newURI(...)

@@ +703,5 @@
> +                            .getService(Ci.nsIIOService);
> +          let uri = IOService.newURI(detail.url, null, null);
> +
> +          let history = Cc["@mozilla.org/browser/proxyhistory;1"]
> +                          .getService(Ci.nsIB2GGlobalHistory2);

make that a LazyServiceGetter

@@ +710,5 @@
> +        }
> +      case 'clear-history':
> +          let history = Cc["@mozilla.org/browser/proxyhistory;1"]
> +                          .getService(Ci.nsIB2GGlobalHistory2);
> +        history.clearVisited();

add |break;|

::: b2g/components/moz.build
@@ +75,5 @@
> +if CONFIG['MOZ_B2G_HISTORY']:
> +    SOURCES += [
> +        'nsB2GHistory.cpp',
> +        'nsB2GProxyHistory.cpp',
> +    ]

Can you move these under b2g/components/history ?
Also, we don't use the `ns` prefix in new files anymore.

::: b2g/components/nsB2GHistory.h
@@ +27,5 @@
> +  /**
> +   * Obtains a pointer that has had AddRef called on it.  Used by the service
> +   * manager only.
> +   */
> +  static nsB2GHistory* GetSingleton();

that should be  already_AddRefed<B2GHistory>

::: configure.in
@@ +5725,5 @@
> +    else
> +        AC_MSG_ERROR([Cannot use MOZ_B2G_HISTORY alongside MOZ_PLACES.])
> +    fi
> +fi
> +AC_SUBST(MOZ_B2G_HISTORY)

I agree with Andrea that we don't need a flag. Just doing the AC_SUBST should be enough.

::: docshell/base/nsIB2GGlobalHistory2.idl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * Provides information about global history to gecko. 

nit: trailing whitespace.

@@ +9,5 @@
> +
> +#include "nsIGlobalHistory2.idl"
> +
> +[scriptable, uuid(d6c8f7f5-36f1-4dcf-a8f4-b54de6957248)]
> +interface nsIB2GGlobalHistory2 : nsIGlobalHistory2

I'm not a fan of the naming here... but nsIGlobalHistory3 isn't great either?

@@ +21,5 @@
> +
> +    /**
> +     * Clear history.
> +     */
> +    void clearVisited();

maybe clearAllVisited() instead?

::: layout/build/nsLayoutModule.cpp
@@ +254,5 @@
> +#ifdef MOZ_B2G_HISTORY
> +#include "nsB2GHistory.h"
> +#include "nsB2GProxyHistory.h"
> +#endif
> +

Please add the module registration in the component implementation file instead of this one.
For instance see https://mxr.mozilla.org/mozilla-central/source/toolkit/components/diskspacewatcher/DiskSpaceWatcher.cpp#134

Comment 63

[[:fabrice] Fabrice Desré]

Comment on attachment 8577356 [details] [diff] [review]
Gecko patch part 2 v3

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

That looks good to me, but I'd like to confirm if we are just growing the database forever? I looks like we don't have any eviction mechanism.

::: b2g/components/nsB2GHistory.cpp
@@ +92,4 @@
>    if (IsChildProcess()) {
>      // Get the visted from the parent process
>      InfallibleTArray<nsString> visited;
>      ChildProcess()->SendReadVisited(&visited);

Since this is sync, we should instanciate the history service in preload.js

Comment 64

[Albert [:albert]]

Attachment: Gecko patch part 1 v4

Changes from comment 62

Comment 65

[Albert [:albert]]

(In reply to Fabrice Desré [:fabrice] from comment #62)
> Comment on attachment 8577355 [details] [diff] [review]
> Gecko patch part 1 v3
> 
> Review of attachment 8577355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: configure.in
> @@ +5725,5 @@
> > +    else
> > +        AC_MSG_ERROR([Cannot use MOZ_B2G_HISTORY alongside MOZ_PLACES.])
> > +    fi
> > +fi
> > +AC_SUBST(MOZ_B2G_HISTORY)
> 
> I agree with Andrea that we don't need a flag. Just doing the AC_SUBST
> should be enough.
> 

Flag is necessary in order to enable/disable b2g history depending on the build environment (b2g, firefox, fennec). Just doing the AC_SUBST doesn't enable history in b2g

Comment 66

[Albert [:albert]]

(In reply to Albert [:albert] from comment #65)
> (In reply to Fabrice Desré [:fabrice] from comment #62)
> > Comment on attachment 8577355 [details] [diff] [review]
> > Gecko patch part 1 v3
> > 
> > Review of attachment 8577355 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: configure.in
> > @@ +5725,5 @@
> > > +    else
> > > +        AC_MSG_ERROR([Cannot use MOZ_B2G_HISTORY alongside MOZ_PLACES.])
> > > +    fi
> > > +fi
> > > +AC_SUBST(MOZ_B2G_HISTORY)
> > 
> > I agree with Andrea that we don't need a flag. Just doing the AC_SUBST
> > should be enough.
> > 
> 
> Flag is necessary in order to enable/disable b2g history depending on the
> build environment (b2g, firefox, fennec). Just doing the AC_SUBST doesn't
> enable history in b2g

When talking about enable/disable I mean the '#if MOZ_B2G_HISTORY' in dom/ipc/ContentChild, dom/ipc/ContentParent and moz.build

Comment 67

[Albert [:albert]]

Attachment: Gecko patch part 1 v4

Sorry, I forgot to remove layout/build/moz.build includes in previous patch.

Comment 68

[Albert [:albert]]

(In reply to Fabrice Desré [:fabrice] from comment #63)
> Comment on attachment 8577356 [details] [diff] [review]
> Gecko patch part 2 v3
> 
> Review of attachment 8577356 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That looks good to me, but I'd like to confirm if we are just growing the
> database forever? I looks like we don't have any eviction mechanism.

You are right, current implementation does not provide a method to limit the size of the database, it is only cleaned if the user clears history.

We can add a timestamp field in the database schema and check mVisitedURIs size each time AddVisited is called from gaia (always in parent process, which is the only process that deals with the db), if the size is greater than a given threshold we will clear a fixed amount of entries depending on the timestamp. Note that then we need to sync childrens through IPC.

What do you think?

> ::: b2g/components/nsB2GHistory.cpp
> @@ +92,4 @@
> >    if (IsChildProcess()) {
> >      // Get the visted from the parent process
> >      InfallibleTArray<nsString> visited;
> >      ChildProcess()->SendReadVisited(&visited);
> 
> Since this is sync, we should instanciate the history service in preload.js

IHistory can not be instantiated from js.

Comment 69

[Martijn Wargers (dead)]

I assume with these patches, the tests in these files can be re-enabled for b2g?
http://mxr.mozilla.org/mozilla-central/source/dom/html/test/mochitest.ini#211
http://mxr.mozilla.org/mozilla-central/source/layout/style/test/mochitest.ini#247

Comment 70

[Andrea Marchesini [:baku]]

> What do you think?

Or we can just have a pref with a max number of visited URLs and when that limit is reach, we remove old URLs based on the sqlite IDs.

Comment 71

[Albert [:albert]]

(In reply to Andrea Marchesini (:baku) from comment #70)
> > What do you think?
> 
> Or we can just have a pref with a max number of visited URLs and when that
> limit is reach, we remove old URLs based on the sqlite IDs.

That's more or less what I said, I proposed to add a timestamp because the current id of the database is the hash of the string. The reason of having the hash as an id was to have a better performance in previous patches in order to avoid to check if the entry was already added. However, current implementation allows to have an incremental id with no loss of performance. So makes sense to change the id cause we save disk usage.

Comment 72

[[:fabrice] Fabrice Desré]

(In reply to Albert [:albert] from comment #71)
> (In reply to Andrea Marchesini (:baku) from comment #70)
> > > What do you think?
> > 
> > Or we can just have a pref with a max number of visited URLs and when that
> > limit is reach, we remove old URLs based on the sqlite IDs.
> 
> That's more or less what I said, I proposed to add a timestamp because the
> current id of the database is the hash of the string. The reason of having
> the hash as an id was to have a better performance in previous patches in
> order to avoid to check if the entry was already added. However, current
> implementation allows to have an incremental id with no loss of performance.
> So makes sense to change the id cause we save disk usage.

Let's do that then!

Comment 73

[Albert [:albert]]

Attachment: Gecko patch part 1 v4

Just rebased

Comment 74

[Albert [:albert]]

Attachment: Gecko patch part 2 v4

Added mechanism to clear the database at a given threshold.

Comment 75

[Andrea Marchesini [:baku]]

Comment on attachment 8592875 [details] [diff] [review]
Gecko patch part 2 v4

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

I think we must definitely do this fully async. This code runs on the main-thread and it involves IO that could block the thread.

::: b2g/components/history/B2GHistory.cpp
@@ +68,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_MOZISTORAGESTATEMENTCALLBACK
> +
> +  explicit ReadDBListener();

no explicit here.

@@ +77,5 @@
> +};
> +
> +NS_IMPL_ISUPPORTS(ReadDBListener, mozIStorageStatementCallback)
> +
> +ReadDBListener::ReadDBListener()

initialize mId

@@ +83,5 @@
> +}
> +
> +NS_IMETHODIMP
> +ReadDBListener::HandleError(mozIStorageError* aError)
> +{

MOZ_ASSERT(aError);

@@ +90,5 @@
> +
> +  nsAutoCString message;
> +  aError->GetMessage(message);
> +
> +

extra line.

@@ +101,5 @@
> +}
> +
> +NS_IMETHODIMP
> +ReadDBListener::HandleResult(mozIStorageResultSet *aResult)
> +{

MOZ_ASSERT(aResult);
MOZ_ASSERT(mUrl.IsEmpty());

@@ +110,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }
> +
> +    nsString url;

nsAutoString

@@ +184,5 @@
> +  }
> +
> +  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +  NS_ENSURE_TRUE(obs, NS_ERROR_FAILURE);
> +  if (NS_FAILED(obs->AddObserver(this, "profile-before-change", false))) {

I don't see where you call RemoveObserver

@@ +207,5 @@
> +    return rv;
> +  }
> +
> +  nsCOMPtr<mozIStorageService> storage = do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID);
> +  if (!storage) {

NS_WARN(!storage)) {

@@ +212,5 @@
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +
> +  rv = storage->OpenDatabase(historyFile, getter_AddRefs(mDBConn));
> +  NS_ENSURE_SUCCESS(rv, rv);

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

as mozIStorageConnection.idl says, you should use openAsyncDatabase:

   * This method MUST be called from the main thread. The connection object
   * returned by this function is not threadsafe. You MUST use it only from
   * the main thread.

@@ +215,5 @@
> +  rv = storage->OpenDatabase(historyFile, getter_AddRefs(mDBConn));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  bool ready;
> +  mDBConn->GetConnectionReady(&ready);

rv = ...
if (NS_WARN_IF(..

@@ +221,5 @@
> +    // delete and try again
> +    rv = historyFile->Remove(false);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    rv = storage->OpenDatabase(historyFile, getter_AddRefs(mDBConn));

same here.

@@ +239,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +
> +  // make operations on the table asynchronous, for performance
> +  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("PRAGMA synchronous = OFF"));

same here: executeSimpleSQLAsync

@@ +281,5 @@
> +    return rv;
> +  }
> +
> +  // create the table
> +  return mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(

executeSimpleSQLAsync

@@ +316,5 @@
> +
> +    nsTArray<ContentParent*> parents;
> +    ContentParent::GetAll(parents);
> +    for (uint32_t i = 0; i < parents.Length(); ++i) {
> +      if (!parents[i]->SendAddVisited(nsString(url))) {

don't send it 1 by 1 but send an array.

::: b2g/components/history/B2GHistory.h
@@ +13,5 @@
>  #include "nsIURI.h"
>  #include "nsTPriorityQueue.h"
>  
> +class mozIStorageConnection;
> +class mozIStorageAsyncStatement;

alphabetic order.

@@ +22,5 @@
>  #define B2GHISTORY_CONTRACTID \
>  "@mozilla.org/browser/history;1"
>  
> +class B2GHistory final : public mozilla::IHistory,
> +                         public nsIObserver {

{ in a new line

@@ +44,5 @@
>  private:
>    B2GHistory();
>    ~B2GHistory();
>  
> +  friend class ReadDBListener;

put it under line 26.

Comment 76

[Albert [:albert]]

Attachment: Gecko patch part 2 v5

Changes from comment 75.

Moved database management from sync to async.

Added 'lazy load' of entries in child processes from parent.

Check if history links are inBrowser elements.

Comment 77

[Andrea Marchesini [:baku]]

Comment on attachment 8599850 [details] [diff] [review]
Gecko patch part 2 v5

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

The main problem I see in this patch is race condition. For instance:

1. child process asks for the list of URIs
2. the parent process B2GHistory is still loading data from SQL (maybe because I/O is slow)
3. child process gets an empty list.

I think a nice way to resolve this issue is to have a pending queue of operations. Any time we receive a getVisited request, we check if the loading from sqlite is finished.
In case it's not, we append a operation task in a list. When sqlite loading is completed, you manage the list in order.
The list has to contain: delete/get/set operations.

The same has to be done for any I/O operation. For instance: freeHistory can take a while. In the meantime all the incoming requests has to be stored in the pending queue.

What do you think about this approach?

::: b2g/components/history/B2GHistory.cpp
@@ +85,5 @@
> +StatementCallback::StatementCallback(B2GHistory::StatementCallbackType aType)
> +  : mType(aType)
> +  , mId(1)
> +{
> +  MOZ_ASSERT(!IsChildProcess());

MOZ_ASSERT(NS_IsMainThread());

add those checks also in the other CTORs.

@@ +93,5 @@
> +StatementCallback::HandleError(mozIStorageError* aError)
> +{
> +  MOZ_ASSERT(aError);
> +
> +  if (B2GHistory::ReadAllStatement) {

if (mType == ... ?

@@ +108,5 @@
> +  nsPrintfCString msg("B2GHistory: Error %d occurred while "
> +                      "reading the databas with message '%s'.", result, message.get());
> +
> +  NS_WARNING(msg.get());
> +  

extra space.

@@ +119,5 @@
> +  MOZ_ASSERT(aResult);
> +
> +  if (mType == B2GHistory::ReadAllStatement) {
> +    nsCOMPtr<mozIStorageRow> row;
> +    nsTArray<nsString> urls;

nsAutoTArray<nsString, 200> urls;

@@ +126,5 @@
> +      nsAutoString url;
> +      nsresult rv = row->GetString(0, url);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return rv; 
> +      }

extra space.

@@ +131,5 @@
> +
> +      urls.AppendElement(url);
> +    }
> +
> +    gB2GHistory->SetVisited(urls);

return NS_OK;

@@ +137,5 @@
> +
> +  if (mType == B2GHistory::ReadOldStatement) {
> +    MOZ_ASSERT(mUrls.IsEmpty());
> +
> +    int64_t id;

id = 0; otherwise, if we don't have any row mId will be set to a random value.

@@ +154,5 @@
> +
> +      mUrls.AppendElement(url);
> +    }
> +
> +    mId = id;

return NS_OK;

@@ +165,5 @@
> +StatementCallback::HandleCompletion(uint16_t aReason)
> +{
> +  if (aReason == REASON_FINISHED) {
> +    switch (mType) {
> +

extra line

@@ +167,5 @@
> +  if (aReason == REASON_FINISHED) {
> +    switch (mType) {
> +
> +      case B2GHistory::SetPragmaStatement:
> +        return gB2GHistory->Read();

instead using gB2GHistory, pass the class to the CTOR and use it as member.

@@ +202,5 @@
> +    NS_DECL_MOZISTORAGECOMPLETIONCALLBACK
> +
> +    DatabaseListener();
> +
> +  protected:

remove this protected.

@@ +204,5 @@
> +    DatabaseListener();
> +
> +  protected:
> +};
> + 

extra space.

@@ +207,5 @@
> +};
> + 
> +NS_IMPL_ISUPPORTS(DatabaseListener, mozIStorageCompletionCallback)
> +
> +DatabaseListener::DatabaseListener()

Move this into the class declaration.

@@ +218,5 @@
> +  if (NS_WARN_IF(NS_FAILED(aResult))) {
> +    return aResult;
> +  }
> +
> +  gB2GHistory->mDBConn = do_QueryInterface(aConnection);

Instead using gB2GHistory, can you take a reference of B2GHistory obj in the CTOR of DatabaseListener?
gB2GHistory could be null if Shutdown is called and I don't know if this Complete() can happen after the Shutdown (in theory no, I guess).

@@ +264,5 @@
>  }
>  
>  nsresult
>  B2GHistory::Init()
>  {

MOZ_ASSERT(NS_IsMainThread());

@@ +277,5 @@
> +    return rv;
> +  }
> +
> +  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +  NS_ENSURE_TRUE(obs, NS_ERROR_FAILURE);

if (NS_WARN_IF(!obs)) {
  return NS_ERROR_FAILURE;
}

@@ +278,5 @@
> +  }
> +
> +  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +  NS_ENSURE_TRUE(obs, NS_ERROR_FAILURE);
> +  if (NS_FAILED(obs->AddObserver(this, "profile-before-change", false))) {

rv = ..
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +304,5 @@
> +  }
> +
> +  nsCOMPtr<mozIStorageService> storage = do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID);
> +  if (NS_WARN_IF(!storage)) {
> +    return NS_ERROR_UNEXPECTED;

maybe NS_ERROR_FAILURE;

@@ +312,5 @@
> +  dbFile->SetAsISupports(historyFile);
> +
> +  nsCOMPtr<DatabaseListener> dbListener = new DatabaseListener();
> +
> +  rv = storage->OpenAsyncDatabase(dbFile, nullptr, dbListener);

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

return NS_OK;

@@ +323,5 @@
> +  // cache frequently used statements (for insertion, deletion, and updating)
> +  nsresult rv = mDBConn->CreateAsyncStatement(NS_LITERAL_CSTRING(
> +    "INSERT INTO visited_urls (id, url) "
> +    "VALUES (?1, ?2)"), getter_AddRefs(mStmtInsert));
> +  NS_ENSURE_SUCCESS(rv, rv);

replace those with if (NS_WARN_IF(...

@@ +328,5 @@
> +
> +  rv = mDBConn->CreateAsyncStatement(NS_LITERAL_CSTRING(
> +    "UPDATE visited_urls SET id = ?1 "
> +    "WHERE url = ?2"), getter_AddRefs(mStmtReplace));
> +  NS_ENSURE_SUCCESS(rv, rv);

here too.

@@ +338,5 @@
> +                                       "ORDER BY id ASC LIMIT ");
> +  query.AppendInt(historyThreshold);
> +
> +  rv = mDBConn->CreateAsyncStatement(query, getter_AddRefs(mStmtOldEntries));
> +  NS_ENSURE_SUCCESS(rv, rv);

here

@@ +342,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = mDBConn->CreateAsyncStatement(NS_LITERAL_CSTRING(
> +    "DELETE FROM visited_urls "
> +    "Where id <= ?1"), getter_AddRefs(mStmtDelete));

if (NS_WARN_IF(...
Otherwise, if this fails we don't have warnings.

return NS_OK;

@@ +355,5 @@
> +    new StatementCallback(B2GHistory::SetPragmaStatement);
> +
> +  // make operations on the table asynchronous, for performance
> +  nsCOMPtr<mozIStoragePendingStatement> pending;
> +  return mDBConn->ExecuteSimpleSQLAsync(NS_LITERAL_CSTRING("PRAGMA synchronous = OFF"),

nsresult rv = ...
if (NS_WARN_IF (..

return NS_OK

@@ +370,5 @@
> +  nsCOMPtr<StatementCallback> callback =
> +    new StatementCallback(B2GHistory::SetVersionStatement);
> +
> +  nsCOMPtr<mozIStoragePendingStatement> pending;
> +  return mDBConn->ExecuteSimpleSQLAsync(stmt, callback, getter_AddRefs(pending));

if (NS_WARN_IF(...

return NS_OK

@@ +397,5 @@
> +  nsCOMPtr<mozIStoragePendingStatement> pending;
> +  nsCOMPtr<StatementCallback> callback =
> +    new StatementCallback(B2GHistory::ReadAllStatement);
> +
> +  return stmt->ExecuteAsync(callback, getter_AddRefs(pending));

if (NS_WARN_IF(...

return NS_OK

@@ +420,5 @@
> +  if (!mIsDataLoaded && IsChildProcess()) {
> +    mIsDataLoaded = true;
> +    ChildProcess()->SendReadVisited();
> +  }
> +  

extra spaces.

@@ +572,5 @@
>  
>  nsresult
> +B2GHistory::UpdateId(const nsAString& aUri)
> +{
> +

extra line

@@ +586,5 @@
> +    return rv;
> +  }
> +
> +  nsCOMPtr<mozIStoragePendingStatement> pending;
> +  return mStmtReplace->ExecuteAsync(nullptr, getter_AddRefs(pending));

if (NS_WARN_IF(...

return NS_OK

@@ +605,5 @@
> +    return rv;
> +  }
> +
> +  nsCOMPtr<mozIStoragePendingStatement> pending;
> +  return mStmtInsert->ExecuteAsync(nullptr, getter_AddRefs(pending));

ditto

@@ +614,5 @@
> +{
> +  nsCOMPtr<mozIStoragePendingStatement> pending;
> +  nsCOMPtr<StatementCallback> callback =
> +    new StatementCallback(B2GHistory::ReadOldStatement);
> +  return mStmtOldEntries->ExecuteAsync(callback, getter_AddRefs(pending));

ditto

@@ +643,5 @@
> +  }
> +
> +  nsCOMPtr<mozIStoragePendingStatement> pending;
> +  return mStmtDelete->ExecuteAsync(nullptr, getter_AddRefs(pending));
> +}

ditto

@@ +684,5 @@
> +  if (mDBConn) {
> +    nsCOMPtr<mozIStorageAsyncStatement> removeStmt;
> +    nsresult rv = mDBConn->CreateAsyncStatement(NS_LITERAL_CSTRING(
> +          "DELETE FROM visited_urls"), getter_AddRefs(removeStmt));
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

why this assert?

@@ +691,5 @@
> +    }
> +
> +    nsCOMPtr<mozIStoragePendingStatement> pending;
> +    rv = removeStmt->ExecuteAsync(nullptr, getter_AddRefs(pending));
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

here too.

::: dom/ipc/ContentParent.cpp
@@ +2590,5 @@
>      B2GHistory* b2gHistory = static_cast<B2GHistory*>(history.get());
>      MOZ_ASSERT(b2gHistory, "We have no b2gHistory in the Chrome process !");
>  
> +    InfallibleTArray<nsString> visited;
> +    b2gHistory->GetVisited(&visited);

In theory this operation can be still pending, right?

Comment 78

[Albert [:albert]]

I'm currently working on other tasks. Leaving it free to let someone else to take it.

Comment 79

[Albert [:albert]]

Attachment: Gecko patch part 2 v6

Changes from comment 77 and added queue for database operations to avoid race conditions.

Comment 81

[BMO Automation]

Firefox OS is not being worked on