Closed Bug 942622 Opened 11 years ago Closed 11 years ago

Intermittent browser_522545.js | sessionstore got correct userTypedValue - Got undefined, expected example.org, | sessionstore got correct userTypedClear - Got undefined, expected 0

Categories

(Firefox :: Session Restore, defect)

Product:
Firefox
Component:
Session Restore
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 28
Tracking Flags:
Tracking Status
firefox26 --- unaffected
firefox27 --- unaffected
firefox28 --- fixed
firefox-esr24 --- unaffected

People

(Reporter: philor, Assigned: ttaubert)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 2 obsolete files)

0001-Bug-942622-Invalidate-TabStateCache-when-.userTypedV.patch
5.44 KB, patch
Yoric
: review+
dao
: review+
Details | Diff | Splinter Review 
https://tbpl.mozilla.org/php/getParsedLog.php?id=31020715&tree=Mozilla-Inbound
WINNT 6.2 mozilla-inbound debug test mochitest-browser-chrome on 2013-11-24 11:26:20 PST for push 5045e4af5807
slave: t-w864-ix-088

11:49:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_522545.js | userTypedClear was not changed when changing gURLBar.value
11:49:25     INFO -  JavaScript strict warning: chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_522545.js, line 209: reference to undefined property newState.windows[0].tabs[0].userTypedValue
11:49:25  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_522545.js | sessionstore got correct userTypedValue - Got undefined, expected example.org
11:49:25     INFO -  Stack trace:
11:49:25     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 705
11:49:25     INFO -      JS frame :: chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_522545.js :: test/test_getBrowserState_userTypedValue/</< :: line 210
11:49:25     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: testScope/test_executeSoon/<.run :: line 734
11:49:25     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
11:49:25  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_522545.js | sessionstore got correct userTypedClear - Got undefined, expected 0
11:49:25     INFO -  Stack trace:
11:49:25     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 705
11:49:25     INFO -      JS frame :: chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_522545.js :: test/test_getBrowserState_userTypedValue/</< :: line 212
11:49:25     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: testScope/test_executeSoon/<.run :: line 734
11:49:25     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
This is spiking hard. Need me to find a regression range, Tim?
Flags: needinfo?(ttaubert)
Pretty sure this is caused by bug 940777.
Blocks: 940777
Flags: needinfo?(ttaubert)
The problem here is that the async collection calls ss.getCurrentState (as could any other code or add-on) and causes the TabStateCache to be filled for the tab used for testing. When checking the serialized .userTypedValue we're just using cached data because that hasn't been invalidated properly.
I will need to ask someone else to review the UserTypedValueChanged event but I first wanted you to sign it off. I don't think we should invalidate when userTypedValue is set to null as that happens when loads stop and that is covered quite well. Also that makes browser_tabStateCache.js tests fail because we over-invalidate.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8337948 - Flags: review?(dteller)
Comment on attachment 8337948 [details] [diff] [review]
0001-Bug-942622-Invalidate-TabStateCache-when-.userTypedV.patch

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

I'm not very familiar with userTypedValue, but the patch looks good to me.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +651,5 @@
>          TabStateCache.onBrowserContentsSwapped(browser, otherBrowser);
>          break;
> +      case "UserTypedValueChanged":
> +        browser = aEvent.currentTarget;
> +        if (browser.userTypedValue) {

So you're not actually using the value sent with the event?
I guess that makes sense in case of several successive change.

::: toolkit/content/widgets/browser.xml
@@ +677,5 @@
> +        <setter><![CDATA[
> +          this.userTypedClear = 0;
> +          this._userTypedValue = val;
> +
> +          let data = {"detail": val};

Maybe "newValue"?
Attachment #8337948 - Flags: review?(dteller) → review+
(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #110)
> > +      case "UserTypedValueChanged":
> > +        browser = aEvent.currentTarget;
> > +        if (browser.userTypedValue) {
> 
> So you're not actually using the value sent with the event?
> I guess that makes sense in case of several successive change.

I could use that. It probably doesn't make a big difference.

> ::: toolkit/content/widgets/browser.xml
> @@ +677,5 @@
> > +        <setter><![CDATA[
> > +          this.userTypedClear = 0;
> > +          this._userTypedValue = val;
> > +
> > +          let data = {"detail": val};
> 
> Maybe "newValue"?

AFAIK we can't change the argument name used in XBL setters. In case you were referring to the "detail" key, that is a field needed by the CustomEvent constructor.
OS: Windows 8 → All
Hardware: x86 → All
Attachment #8337948 - Flags: review?(dao)
Comment on attachment 8337948 [details] [diff] [review]
0001-Bug-942622-Invalidate-TabStateCache-when-.userTypedV.patch

>+          let data = {"detail": val};
>+          let event = new CustomEvent("UserTypedValueChanged", data);

event.detail is unused and redundant. You shouldn't set it.
Attachment #8337948 - Flags: review?(dao) → review-
Carrying over r+ from David. Removed event.detail.
Attachment #8337948 - Attachment is obsolete: true
Attachment #8338593 - Flags: review?(dao)
Comment on attachment 8338593 [details] [diff] [review]
0001-Bug-942622-Invalidate-TabStateCache-when-.userTypedV.patch

>+      case "UserTypedValueChanged":
>+        browser = aEvent.currentTarget;
>+        if (browser.userTypedValue) {
>+          TabStateCache.updateField(browser, "userTypedValue", browser.userTypedValue);
>+          TabStateCache.updateField(browser, "userTypedClear", browser.userTypedClear);
>+        }

I'm not sure I understand the 'if (browser.userTypedValue)' check. If userTypedValue is cleared, doesn't the cache need to be updated too? Or does this happen elsewhere? If so, why not cover it here instead?
(In reply to Dão Gottwald [:dao] from comment #129)
> I'm not sure I understand the 'if (browser.userTypedValue)' check. If
> userTypedValue is cleared, doesn't the cache need to be updated too? Or does
> this happen elsewhere? If so, why not cover it here instead?

userTypedValue is nulled when the load has finished or stopped and that is covered by load and progress listeners elsewhere. It also made browser_tabStateCache.js fail which indicated that we were over-invalidating a lot - so I removed that.
(In reply to Tim Taubert [:ttaubert] from comment #137)
> (In reply to Dão Gottwald [:dao] from comment #129)
> > I'm not sure I understand the 'if (browser.userTypedValue)' check. If
> > userTypedValue is cleared, doesn't the cache need to be updated too? Or does
> > this happen elsewhere? If so, why not cover it here instead?
> 
> userTypedValue is nulled when the load has finished or stopped and that is
> covered by load and progress listeners elsewhere.

Are you talking about browser.userTypedValue or the cache? I was asking why the latter shouldn't be cleared when getting the UserTypedValueChanged event.
(In reply to Dão Gottwald [:dao] from comment #147)
> (In reply to Tim Taubert [:ttaubert] from comment #137)
> > (In reply to Dão Gottwald [:dao] from comment #129)
> > > I'm not sure I understand the 'if (browser.userTypedValue)' check. If
> > > userTypedValue is cleared, doesn't the cache need to be updated too? Or does
> > > this happen elsewhere? If so, why not cover it here instead?
> > 
> > userTypedValue is nulled when the load has finished or stopped and that is
> > covered by load and progress listeners elsewhere.
> 
> Are you talking about browser.userTypedValue or the cache? I was asking why
> the latter shouldn't be cleared when getting the UserTypedValueChanged event.

Yeah I was talking about the cache that is cleared/invalidated when the load stops/finishes. That is done using load and progress listeners. Clearing the cache when setting userTypedValue=null made browser_tabStateCache.js fail because we re-collect data too often.
What if you get rid of the userTypedValue invalidation happening in those load and progress listeners? Is that feasible? The real userTypedValue is also cleared via progress listeners, as far as I remember, so I don't think I see why updating the cache in response to that shouldn't be appropriate.
I can try and find out what exactly made browser_tabStateCache.js fail, maybe userTypedValue is cleared elsewhere where we don't expect it, other than the load and progress listeners?

We mostly invalidate on load and progress to re-read session history entries. We didn't pay attention to the userTypedValue so far (see this bug). In the future, when session history is broadcasted and invalidation happens in the frame script only, we don't have any load listeners and will probably need to invalidate when userTypedValue is cleared as well.
Modified the patch to update the TabStateCache when (userTypedValue == null) as well. That broke browser_tabStateCache.js because there were unexpected cache misses. To fix that I removed the .recordAccess() calls from updateField() and removeField() as I felt they were wrong. We surgically update the cached tab state and do not access it, so we shouldn't record a hit or miss.
Attachment #8338593 - Attachment is obsolete: true
Attachment #8338593 - Flags: review?(dao)
Attachment #8339184 - Flags: review?(dteller)
Attachment #8339184 - Flags: review?(dao)
Attachment #8339184 - Flags: review?(dao) → review+
Comment on attachment 8339184 [details] [diff] [review]
0001-Bug-942622-Invalidate-TabStateCache-when-.userTypedV.patch

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

Looks good to me.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1244,5 @@
>    onTabAdd: function ssi_onTabAdd(aWindow, aTab, aNoNotification) {
>      let browser = aTab.linkedBrowser;
>      browser.addEventListener("load", this, true);
>      browser.addEventListener("SwapDocShells", this, true);
> +    browser.addEventListener("UserTypedValueChanged", this, true);

Maybe we should start treating browser event listeners as notifications, messages, etc. and register/unregister them all from an array. Not necessarily in this bug, though.

::: browser/components/sessionstore/src/TabStateCache.jsm
@@ -235,5 @@
>      let data = this._data.get(key);
>      if (data) {
>        data[aField] = aValue;
>      }
> -    TabStateCacheTelemetry.recordAccess(!!data);

Fair enough.
Attachment #8339184 - Flags: review?(dteller) → review+
(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #293)
> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +1244,5 @@
> >    onTabAdd: function ssi_onTabAdd(aWindow, aTab, aNoNotification) {
> >      let browser = aTab.linkedBrowser;
> >      browser.addEventListener("load", this, true);
> >      browser.addEventListener("SwapDocShells", this, true);
> > +    browser.addEventListener("UserTypedValueChanged", this, true);
> 
> Maybe we should start treating browser event listeners as notifications,
> messages, etc. and register/unregister them all from an array. Not
> necessarily in this bug, though.

Filed bug 944375.
https://hg.mozilla.org/mozilla-central/rev/662e8032dcce
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: