GlobalHistory is accessing Gecko on the wrong thread

RESOLVED FIXED in Firefox 11

Status

()

Firefox for Android
General
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gcp, Assigned: blassey)

Tracking

unspecified
Firefox 12
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, fennec11+)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Updated

6 years ago
Assignee: nobody → blassey.bugs
(Reporter)

Comment 1

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

Updated

6 years ago
tracking-fennec: --- → 11+
Priority: -- → P2
Created attachment 586266 [details] [diff] [review]
patch
Attachment #586266 - Flags: review?(doug.turner)

Updated

6 years ago
Attachment #586266 - Flags: review?(doug.turner) → review+
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.
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+

Comment 5

6 years ago
> 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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdabac538e90
Whiteboard: [inbound]
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?

Comment 9

6 years ago
> 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();
Blocks: 717134
(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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
https://hg.mozilla.org/releases/mozilla-aurora/rev/bd6bcca22234
status-firefox11: --- → fixed
status-firefox12: --- → fixed
Depends on: 718961
You need to log in before you can comment on or make changes to this bug.