Last Comment Bug 739171 - Don't save tabItem data while updating tabItems
: Don't save tabItem data while updating tabItems
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 14
Assigned To: Tim Taubert [:ttaubert]
:
:
Mentors:
Depends on: 739805
Blocks: 707604
  Show dependency treegraph
 
Reported: 2012-03-26 03:11 PDT by Tim Taubert [:ttaubert]
Modified: 2016-04-12 14:00 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (8.71 KB, patch)
2012-03-26 03:11 PDT, Tim Taubert [:ttaubert]
dietrich: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2012-03-26 03:11:01 PDT
Created attachment 609271 [details] [diff] [review]
patch v1

This bug is the cause of bug 707604. It was rather easy to reproduce on my system because all it needed was the right timing of calling tabItem.save() while we're still waiting for SSWindowStateReady. So before session store has even finished restoring a state we're already altering it here:

http://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/tabitems.js#1018

Blocking the whole storage while waiting for SSWindowStateReady or queuing storage calls would be the better solution but as we have good progress with the TabGroups API the storage will soon no longer be taken care of by Panorama itself.

To fix the intermittent orange I had to remove the tabItem.save() call and investigated why we actually need to store the current URL and title of a tabItem ourselves - instead of just reading it from the session store state. This seems more like legacy code to me and so I removed the duplicated url and title data from the code.

Raymond: can you please give me a quick feedback on this patch and whether you see any problems with this approach? Thanks!
Comment 1 Raymond Lee [:raymondlee] 2012-03-26 04:16:08 PDT
(In reply to Tim Taubert [:ttaubert] from comment #0)
> Created attachment 609271 [details] [diff] [review]
> patch v1
> 
> This bug is the cause of bug 707604. It was rather easy to reproduce on my
> system because all it needed was the right timing of calling tabItem.save()
> while we're still waiting for SSWindowStateReady. So before session store
> has even finished restoring a state we're already altering it here:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/
> tabitems.js#1018
> 
> Blocking the whole storage while waiting for SSWindowStateReady or queuing
> storage calls would be the better solution but as we have good progress with
> the TabGroups API the storage will soon no longer be taken care of by
> Panorama itself.
> 
> To fix the intermittent orange I had to remove the tabItem.save() call and
> investigated why we actually need to store the current URL and title of a
> tabItem ourselves - instead of just reading it from the session store state.
> This seems more like legacy code to me and so I removed the duplicated url
> and title data from the code.
> 
> Raymond: can you please give me a quick feedback on this patch and whether
> you see any problems with this approach? Thanks!

I have a quick look and I don't see any issues with the patch.  I am not sure why we don't get the url and title from session store state too.
Comment 2 Tim Taubert [:ttaubert] 2012-03-26 04:21:48 PDT
(In reply to Raymond Lee [:raymondlee] from comment #1)
> > Raymond: can you please give me a quick feedback on this patch and whether
> > you see any problems with this approach? Thanks!
> 
> I have a quick look and I don't see any issues with the patch.  I am not
> sure why we don't get the url and title from session store state too.

Thank you for the quick feedback!
Comment 3 Dietrich Ayala (:dietrich) 2012-03-26 15:01:02 PDT
Comment on attachment 609271 [details] [diff] [review]
patch v1

Review of attachment 609271 [details] [diff] [review]:
-----------------------------------------------------------------

r=me w/ answers or changes to these comments.

::: browser/components/tabview/storage.js
@@ +125,5 @@
>  
>    // ----------
> +  // Function: getTabState
> +  // Returns the current state of the given tab.
> +  // Saves the data for a single groupItem, associated with a specific window.

what is this sentence referring to? I don't see any saving going on.

::: browser/components/tabview/tabitems.js
@@ +204,5 @@
>      this._cachedImageData = imageData;
>      this.$cachedThumb.attr("src", this._cachedImageData).show();
>      this.$canvas.css({opacity: 0});
> +    let title = this.getTabStateUrl();
> +    let label = this.getTabStateTitle();

using 'title' for the url is a little confusing. might be clearer to call it 'url' so that the reasoning behind how it's used here is more explicit.

how often is showCachedData called? you're having to cross into session restore code twice every time, for exactly the same data. maybe getTabProperties or something like that, which returns object w/ properly massaged title, url, etc?
Comment 4 Tim Taubert [:ttaubert] 2012-03-27 03:26:22 PDT
(In reply to Dietrich Ayala (:dietrich) from comment #3)
> > +  // Function: getTabState
> > +  // Returns the current state of the given tab.
> > +  // Saves the data for a single groupItem, associated with a specific window.
> 
> what is this sentence referring to? I don't see any saving going on.

This slipped in there somehow, removed.

> > +    let title = this.getTabStateUrl();
> > +    let label = this.getTabStateTitle();
> 
> using 'title' for the url is a little confusing. might be clearer to call it
> 'url' so that the reasoning behind how it's used here is more explicit.

Right, fixed.

> how often is showCachedData called? you're having to cross into session
> restore code twice every time, for exactly the same data. maybe
> getTabProperties or something like that, which returns object w/ properly
> massaged title, url, etc?

It's not called too often (once per restored tab) but I also think it makes sense to add a .getTabState() method that returns both.
Comment 5 Tim Taubert [:ttaubert] 2012-03-27 04:09:43 PDT
https://hg.mozilla.org/integration/fx-team/rev/26051ffdbc34
Comment 6 Tim Taubert [:ttaubert] 2012-03-27 06:10:50 PDT
Backed out because of session store failures:

https://hg.mozilla.org/integration/fx-team/rev/fb41b10cd782

I'm rather clueless why this patch should make SS tests fail, will investigate.

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_393716.js | also duplicated text data - Got , expected Another value: 1332849265702
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_408470.js | restored stylesheet media_empty
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_408470.js | restored stylesheet media_all
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_408470.js | restored stylesheet media_ALL
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_408470.js | restored stylesheet media_screen
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_408470.js | restored stylesheet media_print_screen
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_408470.js | disabled all stylesheets - Got false, expected true
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_463206.js | text isn't reused for frames - Didn't expect , but got it
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_463206.js | text containing | and # is correctly restored - Didn't expect , but got it
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_466937.js | normal case: file path was correctly preserved - Got , expected /tmp/466937_test.file
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_485482.js | generated XPath expression was valid - Got , expected 0.8427489566229125
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_485482.js | generated XPath expression was valid
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_form_restore_events.js | input events were only dispatched for modified input, textarea fields - Got , expected modify01 modify02 modify03 modify04 modify05
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_form_restore_events.js | change events were only dispatched for modified select, checkbox, radio fields - Got , expected modify06 modify07 modify08 modify09 modify11
Comment 7 Tim Taubert [:ttaubert] 2012-03-28 00:18:02 PDT
https://hg.mozilla.org/integration/fx-team/rev/77f73d3a0d47
Comment 8 Tim Taubert [:ttaubert] 2012-03-29 02:53:24 PDT
https://hg.mozilla.org/mozilla-central/rev/77f73d3a0d47

Note You need to log in before you can comment on or make changes to this bug.