Closed Bug 715507 Opened 8 years ago Closed 8 years ago

GlobalHistory is accessing Gecko on the wrong thread

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 12
Tracking Status
firefox11 --- fixed
firefox12 --- fixed
fennec 11+ ---

People

(Reporter: gcp, Assigned: blassey)

References

Details

(Whiteboard: [inbound])

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → blassey.bugs
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?
tracking-fennec: --- → 11+
Priority: -- → P2
Attached patch patch (obsolete) — Splinter Review
Attachment #586266 - Flags: review?(doug.turner)
Attachment #586266 - Flags: review?(doug.turner) → review+
Attached patch patchSplinter Review
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.
Attachment #586266 - Attachment is obsolete: true
Attachment #586272 - Flags: review?(bugmail.mozilla)
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.
Attachment #586272 - Flags: review?(bugmail.mozilla) → review+
> 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)
(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 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
Attachment #586272 - Flags: approval-mozilla-aurora?
> 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!
(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();
(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 on attachment 586272 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #586272 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/bdabac538e90
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Depends on: 718961
You need to log in before you can comment on or make changes to this bug.