605219 - "Error in console Error: uncaught exception: Not Loading!" after going to a webpage in a tab that failed to load; web page icon fails to load.

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101018 Firefox/4.0b8pre Fennec/4.0b2pre

1. go to URL : http://people.mozilla.com/~nhirata/html_tp
2. double tap to zoom and double tap to unzoom
3. click on the Parent Directory link
4. after it takes a while to try to load, hit the reload button.
5. go to the error console.

Expected: No Errors
Actual: sometimes you may get an error stating 
"Error: uncaught exception: Not Loading!"

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Note : 
This doesn't repro all the time.  I'm not 100 % sure of the conditions required to get this exception.

Comment 2

[Aaron Train [:aaronmt]]

fwiw loading this website and attempting to zoom with double tap causes the same build to crash on my Milestone

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I figured out at least one instance a repro of the "Error: uncaught exception: Not Loading!", retitled bug.

Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101019 Firefox/4.0b8pre Fennec/4.0b2pre
Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b8pre) Gecko/20101019 Firefox/4.0b8pre Fennec/4.0b2pre

1) go to http://www.xkcd.com
2) after load, go to http://www.xkcd.xom ( an unknown web page)
3) after error page loads, go to http://www.google.com
4) go to error console

Expected: web icon loads on step 3 when google finishes loading, no errors
Actual: web icon does not load on step 3 when google finishes loading, error in console : "Error: uncaught exception: Not Loading!"

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Wes - lets find out of this is a real bug or not.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch

Not exactly sure about this patch. Going to think about it a bit more, but figured I'd get some feedback too.

The problem is that we are depending on getting events in a certain order. In:

http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#2200

we depend on knowing the windowID or our browser, but that only changes when we get location change events:

http://mxr.mozilla.org/mobile-browser/source/chrome/content/bindings/browser.xml#268

In this case1 the windowID changes, we get an onStateChange event, and then we get an onLocationChange event. Since we have the wrong windowID during the onStateChange event, we ignore it.

The patch just makes us update the windowID or our browser during both events. I haven't thought this code through enough to know if that is legal or not, but if it is, we should probably update our windowID during all the events here, and if we do that... I'm not even sure if the windowID check is still necessary...

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 486186 [details] [diff] [review]
Patch

I'm not opposed to using this approach. We should try to set the browser.contentWindowId as soon as possible anyway.

Make sure we don't regress any of our browser-chrome tests.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Doug or Joel might be interested in this patch as a way to stop the exceotion from being thrown during a talos Tp4 run.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 486755 [details] [diff] [review]
Patch

Have you run this through browser-chrome tests?

Comment 10

[Wesley Johnston (:wesj)]

Whoops. Slipped my mind.

Ran this through with and without the patch. I get failures in both cases. 13 with the patch. 17 without. Most seem fairly unrelated. I'll try to look into them in the morning and figure out why they're failing, which are random, etc. I think a bunch of them are related to the new theme landing.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch v2

This is a cleaner way to do this. I ran this through browser-chrome tests with my fixes in Bug 608349 applied and see zero errors.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 486989 [details] [diff] [review]
Patch v2

nit: _updateID -> updateWindowID

r+ with that change

Comment 13

[Matt Brubeck (:mbrubeck)]

(In reply to comment #12)
> Comment on attachment 486989 [details] [diff] [review]
> Patch v2
> 
> nit: _updateID -> updateWindowID

Done and pushed: http://hg.mozilla.org/mobile-browser/rev/9f81b1160bd2

Comment 14

[Ayan Shah[:ashah]]

verified fixed on:
Mozilla/5.0(Android; Linux armv7l;rv2.0b8pre) Gecko/20101102 Firefox/4.0b8pre
Fennec/4.0b3pre

And

Mozilla/5.0(Maemo; Linux armv7l;rv2.0b8pre) Gecko/20101102 Firefox/4.0b8pre
Fennec/4.0b3pre