Intermittent failure in browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | correct URL for normal mode - Got about:blank, expected about:robots

RESOLVED FIXED

Status

defect
P3
normal
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: Ehsan, Assigned: iangilman)

Tracking

({intermittent-failure})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1287084415.1287085150.14449.gz
Rev3 Fedora 12x64 mozilla-central opt test mochitest-other on 2010/10/14 12:26:55

TEST-START | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | we start with 1 tab
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | we now have 2 tabs
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | Tab View is visible
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | Tab View is no longer visible
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | private browsing is on
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | we have 1 tab in private browsing
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | correct URL for private browsing
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | Tab View is visible again
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | Console message:
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | private browsing is off
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | we have 2 tabs in normal mode
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | correct URL for normal mode
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | correct URL for normal mode
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | Tab View is not visible
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | Tab View is still not visible
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | private browsing is on
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | we have 1 tab in private browsing
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | correct URL for private browsing
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | Console message:
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | private browsing is off
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | we have 2 tabs in normal mode
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | correct URL for normal mode
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | correct URL for normal mode - Got about:blank, expected about:robots
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | we finish with one tab
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | we finish with private browsing off
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | we finish with Tab View not visible
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | Test took 1.585s to complete
Assignee: nobody → ian
OS: Linux → Windows CE
Priority: -- → P3
This failure (expecting "about:robots" but getting "about:blank") seems like we're not waiting long enough for the tabs to load. The routine we use to wait is this: 

function afterAllTabsLoaded(callback) {
  let stillToLoad = 0; 
  function onLoad() {
    this.removeEventListener("load", onLoad, true);
    
    stillToLoad--;
    if (!stillToLoad)
      callback();
  }

  for (let a = 0; a < gBrowser.tabs.length; a++) {
    let browser = gBrowser.tabs[a].linkedBrowser;
    if (browser.webProgress.isLoadingDocument) {
      stillToLoad++;
      browser.addEventListener("load", onLoad, true);
    }
  }
}

Ehsan, any thoughts on what may be going wrong? Is it possible we have a race condition here? If so, how would we fix it?

Maybe .isLoadingDocument isn't a reliable way of knowing whether the tab still needs to load? What if it needs to load but hasn't started loading yet?
The problem with this function is that it doesn't check which load event it's handling.  The PB service internally opens an about:blank tab first and then restores the session, so I'm suspecting that you're getting that load event instead of the load event that you expect to receive.

Why don't you just listen for private-browsing-transition-complete?
(In reply to comment #6)
> The problem with this function is that it doesn't check which load event it's
> handling.  The PB service internally opens an about:blank tab first and then
> restores the session, so I'm suspecting that you're getting that load event
> instead of the load event that you expect to receive.

But we're only subscribing to the tabs we know about; we'd never get the load event for an additional tab. 

I suppose not checking which load event would be a problem if we got multiple load events for the same tab, but even then we're removing the handler after every event, so not even that should happen, right?

> Why don't you just listen for private-browsing-transition-complete?

Does this fire after all of the tabs have finished loading? We need them to finish loading before we continue the test. 

Also, it would be nice to have a nice solid afterAllTabsLoaded function if we can, as it's something we can make use of in other tests as well.
Blocks: 597043
(In reply to comment #8)
> (In reply to comment #6)
> > The problem with this function is that it doesn't check which load event it's
> > handling.  The PB service internally opens an about:blank tab first and then
> > restores the session, so I'm suspecting that you're getting that load event
> > instead of the load event that you expect to receive.
> 
> But we're only subscribing to the tabs we know about; we'd never get the load
> event for an additional tab. 

Session store reuses already open tabs, so you never know!

> I suppose not checking which load event would be a problem if we got multiple
> load events for the same tab, but even then we're removing the handler after
> every event, so not even that should happen, right?

Yes, and since you're not checking for the location in the load event, you don't really know which one you've handled.

> > Why don't you just listen for private-browsing-transition-complete?
> 
> Does this fire after all of the tabs have finished loading? We need them to
> finish loading before we continue the test. 

Yes.

> Also, it would be nice to have a nice solid afterAllTabsLoaded function if we
> can, as it's something we can make use of in other tests as well.

Well, you can use sessionstore-browser-state-restored for other tests.
(In reply to comment #9)
> > > Why don't you just listen for private-browsing-transition-complete?
> > 
> > Does this fire after all of the tabs have finished loading? We need them to
> > finish loading before we continue the test. 
> 
> Yes.

Actually, it doesn't fire after all the load events have been dispatched.  It is fired after the restoration process is finished.  And I assume that you can listen to load event handlers from that point if needed.
Posted patch patch v1Splinter Review
Ok, now waiting for private-browsing-transition-complete before waiting for tabs to load.

As a side note, should we be observing private-browsing-transition-complete in our main PB handling code in tabview/ui.js? Right now we're observing private-browsing-change-granted, private-browsing, and sessionstore-browser-state-restored.
Attachment #484513 - Flags: feedback?(ehsan)
Status: NEW → ASSIGNED
(In reply to comment #12)
> As a side note, should we be observing private-browsing-transition-complete in
> our main PB handling code in tabview/ui.js? Right now we're observing
> private-browsing-change-granted, private-browsing, and
> sessionstore-browser-state-restored.

You should be observing private-browsing-transition-complete instead of sessionstore-browser-state-restored.  Please file a separate bug for that though.
Comment on attachment 484513 [details] [diff] [review]
patch v1

r=me (as the owner of the PB module).
Attachment #484513 - Flags: feedback?(ehsan) → review+
(In reply to comment #13)
> You should be observing private-browsing-transition-complete instead of
> sessionstore-browser-state-restored.  Please file a separate bug for that
> though.

Done. Bug 605935.
http://hg.mozilla.org/mozilla-central/rev/5b36ff18e55a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [orange] → [orange][qa-]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
OS: Windows CE → Linux
Moving to b9
Blocks: 598154
No longer blocks: 597043
Depends on: 623330
bugspam (moving b9 to b10)
Blocks: 608028
bugspam (removing b9)
No longer blocks: 598154
I believe this is fixed by a combination of bug 624265 and bug 610242.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [orange][qa-] → [qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.