Closed Bug 741712 Opened 8 years ago Closed 8 years ago

Fixup URIs stop throbber prematurely

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- beta+

People

(Reporter: kats, Assigned: bnicholson)

Details

Attachments

(1 file, 1 obsolete file)

STR:
1. Start Fennec
2. load some page (e.g. yahoo.com)
3. Tap on the awesome bar to bring up the url entry screen
4. Type in "gregmurray.com" and hit "go" (or enter or whatever)

Expected results:
Either some website loads, or an error message is displayed

Actual results:
The tab is shown rendering yahoo.com, and the URL bar says gregmurray.com.

(Note: there is no website at gregmurray.com; in fact pinging gregmurray.com returns an "Unknown host" error. I suspect we're not handling this error condition properly)

Tested on Galaxy Nexus with recent m-c build.
Assignee: nobody → bnicholson
blocking-fennec1.0: ? → beta+
When trying these STR, I eventually see a connection timeout page after 20-30 seconds or so. This suggests that we're not actually failing, though it appears we are since the spinner is stopped long before this page appears.
(In reply to Mark Finkle (:mfinkle) from comment #2)
> I assume we are not getting a load error? 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#538
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#1555
> 
> Are we getting any state changes?
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#1983

No load errors. We are getting state changes - twice. It looks like we're using THIRD_PARTY_FIXUP to change http://gregmurray.com/ -> http://www.gregmurray.com/.

In general, when we do third party fixup, we receive this order of state change events:
1) document start (original uri)
2) document start (fixup uri)
3) document stop (original uri)
4) document stop (fixup uri)

Steps 2 and 3 happen quickly after we begin the page load. Step 4 happens when the document finishes loading, as expected. We stop the throbber on document stop, so we're stopping it early, at the stop for the original uri (step 3).

This happens for every fixup URI (though usually less noticeable). For instance, type "banana" in the awesomebar, click Go, and you'll notice that the spinner stops before Google loads. If you go to http://www.gregmurray.com/ directly, there won't be a URI fixup, and the spinner stays until there's a timeout.

That said, I'm not sure what the best solution would be. We could increment a counter for document starts and decrement for stops, and only stop the spinner when the counter is 0, but that seems messy.
Summary: Loading a page sometimes silently fails but leaves the new URL in the URL bar → Fixup URIs stop throbber prematurely
Attached patch patch (obsolete) — Splinter Review
The nsIWebProgress lets us know whether the document is loading. If we get a document stop, but the document is still loading, the document stop is ignored.
Attachment #612024 - Flags: review?(mark.finkle)
I'm not a big fan of the approach. I don't like having to check too many booleans everywhere.

Is it possible to check the isDocumentLoading property in JS and just not send the Document Stop? Does that break anything else? Since Java never cares for a Document Stop if the doc is still loading, let's avoid sending it.
Attached patch patch v2Splinter Review
Sure, that should work too.
Attachment #612024 - Attachment is obsolete: true
Attachment #612024 - Flags: review?(mark.finkle)
Attachment #612056 - Flags: review?(mark.finkle)
Comment on attachment 612056 [details] [diff] [review]
patch v2

i like it
Attachment #612056 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/8d7b5db99603
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Throbber stops when the loaded page displays.

Verified fixed on build: Firefox 14.0a1 (2012-04-18)
Device: LG Optimus 2X (Android 2.2.2)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.