Last Comment Bug 715507 - GlobalHistory is accessing Gecko on the wrong thread
: GlobalHistory is accessing Gecko on the wrong thread
Status: RESOLVED FIXED
[inbound]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P2 normal (vote)
: Firefox 12
Assigned To: Brad Lassey [:blassey] (use needinfo?)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on: 718961
Blocks: 717134
  Show dependency treegraph
 
Reported: 2012-01-05 07:55 PST by Gian-Carlo Pascutto [:gcp]
Modified: 2012-01-23 17:24 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
11+


Attachments
patch (4.56 KB, patch)
2012-01-05 16:11 PST, Brad Lassey [:blassey] (use needinfo?)
doug.turner: review+
Details | Diff | Splinter Review
patch (4.55 KB, patch)
2012-01-05 16:28 PST, Brad Lassey [:blassey] (use needinfo?)
bugmail: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Gian-Carlo Pascutto [:gcp] 2012-01-05 07:55:56 PST
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] 2012-01-05 08:10:26 PST
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?) 2012-01-05 16:11:48 PST
Created attachment 586266 [details] [diff] [review]
patch
Comment 3 Brad Lassey [:blassey] (use needinfo?) 2012-01-05 16:28:43 PST
Created attachment 586272 [details] [diff] [review]
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 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-01-05 19:04:02 PST
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) 2012-01-05 20:36:29 PST
> 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 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-01-05 20:51:24 PST
(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?) 2012-01-10 14:38:25 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdabac538e90
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2012-01-10 14:40:59 PST
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) 2012-01-10 16:38:57 PST
> 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?) 2012-01-10 16:40:41 PST
(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 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 18:51:45 PST
(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] 2012-01-11 13:39:55 PST
Comment on attachment 586272 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora.
Comment 13 Ed Morley [:emorley] 2012-01-11 18:22:24 PST
https://hg.mozilla.org/mozilla-central/rev/bdabac538e90
Comment 14 Brad Lassey [:blassey] (use needinfo?) 2012-01-20 13:44:48 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/bd6bcca22234

Note You need to log in before you can comment on or make changes to this bug.