Closed Bug 739171 Opened 12 years ago Closed 12 years ago

Don't save tabItem data while updating tabItems

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 14

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

Attachments

(1 file)

Attached patch patch v1Splinter Review
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!
Attachment #609271 - Flags: feedback?(raymond)
(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.
(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!
Attachment #609271 - Flags: feedback?(raymond) → review?(dietrich)
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?
Attachment #609271 - Flags: review?(dietrich) → review+
(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.
https://hg.mozilla.org/integration/fx-team/rev/26051ffdbc34
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
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
Whiteboard: [fixed-in-fx-team]
Depends on: 739805
https://hg.mozilla.org/mozilla-central/rev/77f73d3a0d47
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: