715507 - GlobalHistory is accessing Gecko on the wrong thread

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Gian-Carlo Pascutto [:gcp]]

See:
https://bugzilla.mozilla.org/show_bug.cgi?id=711034#c6

GlobalHistory is doing native calls in the following construction:
GeckoAppShell.getHandler().new Runnable(....GeckoAppShell.notifyUriVisited())

Which isn't actually the Gecko Thread.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

The native code in toolkit/components/places/nsAndroidHistory.cpp has:

  if (! sHistory)
    return;
  sHistory->mPendingURIs.Push(aUriString);
  NS_DispatchToMainThread(sHistory);

So it's dispatching itself to the main thread. The problem might be that the sHistory->foo call also needs to be on the main thread then?

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

fixed the spelling errors.

Kats, GeckoHistory has some comments about synchronization not being needed because this stuff is run on the same thread. Can you look this and make sure I'm not breaking it.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 586272 [details] [diff] [review]
patch

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

Looks fine overall. A couple of things that can be done as follow-ups.

::: mobile/android/base/GeckoEvent.java
@@ +248,5 @@
>  
> +    public GeckoEvent(int type, String data) {
> +        mType = type;
> +        mCharacters = data;
> +    }

Replacing uses of the GeckoEvent(String) constructor to use this one instead would be good for a follow-up patch.

::: widget/android/nsAppShell.cpp
@@ +453,5 @@
> +#ifdef MOZ_ANDROID_HISTORY
> +        nsAndroidHistory::NotifyURIVisited(nsString(curEvent->Characters()));
> +#endif
> +        break;
> +    }

Since this chunk runs on the gecko thread, you can also change the nsAndroidHistory::NotifyURIVisited implementation to be much simpler. It doesn't have to re-dispatch to the main thread and use the mPendingURIs queue. But this should work fine without that change too, just a little less efficiently.

Comment 5

[Doug Turner (:dougt)]

> Replacing uses of the GeckoEvent(String) constructor to use this one instead would be good for a follow-up patch.

Why?  Isn't this more common:  public GeckoEvent(String uri)

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(In reply to Doug Turner (:dougt) from comment #5)
> > Replacing uses of the GeckoEvent(String) constructor to use this one instead would be good for a follow-up patch.
> 
> Why?  Isn't this more common:  public GeckoEvent(String uri)

I think all of the GeckoEvent constructors should either specify the event type, or none of them should (more consistency in code == less errors). With the "none of them" approach different events with the same data types will have the same constructor signatures with no way to distinguish between them. So all constructors should just take a type.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdabac538e90

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 586272 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
fixes threading warning, so potential thread safety issue
Testing completed (on m-c, etc.): 
on inbound now, I'd suggest a day or two on m-c before uplifting
Risk to taking this patch (and alternatives if risky):
relatively low risk, more of the code path runs on the gecko thread than it did before

Comment 9

[Doug Turner (:dougt)]

> I think all of the GeckoEvent constructors should either specify the event type, or none of them should (more consistency in code == less errors)

Totally in agreement!

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Doug Turner (:dougt) from comment #9)
> > I think all of the GeckoEvent constructors should either specify the event type, or none of them should (more consistency in code == less errors)
> 
> Totally in agreement!

or, should be of the pattern GeckoEvent.createXxxxxxEvent();

Comment 11

[Kartikaya Gupta (email:[e-mail])]

(In reply to Brad Lassey [:blassey] from comment #10)
> 
> or, should be of the pattern GeckoEvent.createXxxxxxEvent();

Even better. I filed bug 717134 so we don't forget about it.

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 586272 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora.

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bdabac538e90

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd6bcca22234