Closed Bug 1083167 Opened 5 years ago Closed 5 years ago

FormHistory.jsm: "update op='bump' does not correctly reference a entry" for empty search terms

Categories

(Firefox :: Search, defect)

33 Branch
defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 36
Iteration:
36.2
Tracking Status
firefox33 --- wontfix
firefox34 --- verified
firefox35 --- verified
firefox36 --- verified

People

(Reporter: toadyshadow101, Assigned: adw)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141011015303

Steps to reproduce:

Open firefox 33
On about:home page (Firefox start page) press the search button with no text in the search field.


Actual results:

[Exception... "update op='bump' does not correctly reference a entry."  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://gre/modules/FormHistory.jsm :: formHistoryUpdate :: line 885"  data: no]

ContentSearch.jsm line:142


Expected results:

not thrown an error
I don't see that error.

Could test:
1) in safe mode: https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode
2) with a clean profile: https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles

Does it fix it?
Flags: needinfo?(toadyshadow101)
Hi loic

Please look in the browser console, This was tested on a clean profile on a fresh installation of windows 7 x64 os.

When restarting in safe mode to test the browser became unresponsive and started lagging by about 6 seconds when dragging the window and activating Firefox menus.

This error in safe mode appeared in the browser console:
TypeError: asm.js type error: Disabled by javascript.options.asmjs in about:config about:home

The above reported error also appeared when pressing the search button with no text in the search field.
Flags: needinfo?(toadyshadow101)
I reproduced it.

Regression range:
good=2014-08-02
bad=2014-08-03
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a5a720259d79&tochange=e6614d8d85f9

Suspected bug: Bug 612453
Blocks: 612453
Component: Untriaged → Search
Flags: needinfo?(adw)
Keywords: regression
cc Bryan for awareness

Does the page still submit the search? i.e. Is the issue only that an error is thrown or is this visible by broken functionality when searching from about:home?
Flags: needinfo?(toadyshadow101)
If a "bump" doesn't work then it means we're aren't recording the search term in form history (either adding an entry if it's new or updating the last use timestamp + use count if it's an existing form history entry).

The code causing this is:
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/FormHistory.jsm?rev=0853b374d32a&mark=855-858,880-881,886-890#844

change.value is falsy on line 857 so we consider this an invalid change that we don't care about. I don't think it's useful/wanted to store an empty string in form history. We didn't allow empty string searches in 31 either so I don't think this is a regression.

Lawrence, since this is only for empty searches and it's just a console warning and not a regression, I don't think this needs tracking for any version.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(adw)
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: FormHistory.jsm Exception about:home search → FormHistory.jsm: "update op='bump' does not correctly reference a entry" for empty search terms
Hi Lawrence

Yes, Searching and the keyword drop down function correctly, The error only logs to console if you press the search button when the text input is empty, Same on the about:newtab search.
Flags: needinfo?(toadyshadow101)
In that case, this issue is not significant enough to track but is something that we would certainly take a fix for.
Attached patch patch (obsolete) — Splinter Review
This seems consistent with search.xml's behavior:
http://hg.mozilla.org/mozilla-central/annotate/c0ddb1b098ec/browser/components/search/content/search.xml#l483
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #8512824 - Flags: review?(adw)
It's a simple fix.

This also replaces the msg.target.contentWindow check with a currentURI check.  We used to check contentWindow here before calling PrivateBrowsingUtils.isWindowPrivate(msg.target.contentWindow), but that was changed to PrivateBrowsingUtils.isBrowserPrivate(msg.target) by http://hg.mozilla.org/mozilla-central/diff/324798b60ba3/browser/modules/ContentSearch.jsm.
Assignee: gavin.sharp → adw
Attachment #8512825 - Flags: review?(MattN+bmo)
Whoops, sorry Gavin.  That's what I get for having the attach-new-patch page open since yesterday and not refreshing the main bug page. :-/
Assignee: adw → gavin.sharp
Comment on attachment 8512824 [details] [diff] [review]
patch

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

Oh, you posted your patch only 30 seconds before I posted mine.  I don't feel so bad now. :-P :-)

I like my patch better... it patches code closer to the problem, so any ContentSearch client sending the AddFormHistoryEntry message with an empty string will work correctly.  Plus it fixes the now unnecessary contentWindow CPOW getter.

So is it OK if I leave my r? for Matt?  Or do you have an opinion on my patch?
Attachment #8512824 - Flags: review?(adw)
Comment on attachment 8512825 [details] [diff] [review]
Don't pass an empty string to FormHistory.update

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

Could you add more context to your patch in the future? 3 lines isn't enough.

::: browser/modules/ContentSearch.jsm
@@ +276,5 @@
> +    // isBrowserPrivate() assumes that the passed-in browser is alive, but due
> +    // to the asynchronous nature of messaging, that may not be the case here.
> +    // If the browser doesn't have a currentURI, skip the call.
> +    if (!entry ||
> +        !msg.target.currentURI ||

So the currentURI check is now to make sure the page is still alive? I don't see why we need that since I don't see why it should prevent use from adding to form history.
FWIW, I also like adw's patch better because of where it's fixing the problem.
(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #13)
> Could you add more context to your patch in the future? 3 lines isn't enough.

Sorry, instead of generating a diff -U8 like usual, I attached the patch right from .hg/patches, which doesn't have a lot of context apparently.

> So the currentURI check is now to make sure the page is still alive? I don't
> see why we need that since I don't see why it should prevent use from adding
> to form history.

It's needed to short-circuit the PrivateBrowsingUtils.isBrowserPrivate(msg.target) call.  If the page is in a private browsing window, we skip adding to form history.  If the browser was destroyed before the message reached ContentSearch, then PrivateBrowsingUtils won't be able to tell whether the browser is private -- actually, PrivateBrowsingUtils.isBrowserPrivate will end up throwing an exception at least in the non-e10s case because browser.contentWindow will be undefined.

If we can't tell whether the browser is private, then assume it is and skip adding to form history.
(In reply to Drew Willcoxon :adw from comment #15)
> actually, PrivateBrowsingUtils.isBrowserPrivate will end up throwing an
> exception at least in the non-e10s case because browser.contentWindow will
> be undefined.

Maybe I should just try-catch the call then?
Stealing bug from Gavin as we discussed on IRC.

This also uses entry === "" instead of !entry so that we still log errors in cases where entry is falsey but not a string, as you requested on IRC.

(In reply to Drew Willcoxon :adw from comment #15)
> Sorry, instead of generating a diff -U8 like usual, I attached the patch
> right from .hg/patches, which doesn't have a lot of context apparently.

Ah, you can set unified=8 in the [diff] section of your .hgrc to fix that.
Assignee: gavin.sharp → adw
Attachment #8512824 - Attachment is obsolete: true
Attachment #8512825 - Attachment is obsolete: true
Attachment #8512825 - Flags: review?(MattN+bmo)
Attachment #8512858 - Flags: review?(MattN+bmo)
Iteration: --- → 36.2
Points: --- → 2
Flags: firefox-backlog+
Comment on attachment 8512858 [details] [diff] [review]
Don't pass an empty string to FormHistory.update + try-catch isBrowserPrivate()

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

::: browser/modules/ContentSearch.jsm
@@ +279,5 @@
> +      // properties, which won't be true if the browser has been destroyed.
> +      // That may be the case here due to the asynchronous nature of messaging.
> +      isPrivate = PrivateBrowsingUtils.isBrowserPrivate(msg.target);
> +    }
> +    catch (err) {}

Nit: cuddle the catch:
} catch (err) {}
Attachment #8512858 - Flags: review?(MattN+bmo) → review+
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
https://hg.mozilla.org/mozilla-central/rev/8546940128b3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
QA Contact: petruta.rasa
Attached patch Beta/34 patchSplinter Review
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #7)
> In that case, this issue is not significant enough to track but is something
> that we would certainly take a fix for.

The m-c patch didn't apply cleanly because the PrivateBrowsingUtils.isWindowPrivate -> isBrowserPrivate caller change landed on 35 (Aurora), in bug 1069059.  This patch still calls the newer isBrowserPrivate, not isWindowPrivate, because that is available on 34 (Beta) due to https://hg.mozilla.org/releases/mozilla-beta/rev/34c81d98d96a.

Approval Request Comment
[Feature/regressing bug #]:
Search suggestions on about:home, bug 612453

[User impact if declined]:
A harmless error reported in the browser console when you click the Search button on about:home/newtab with an empty search box

[Describe test coverage new/current, TBPL]:
We have automated tests for this feature, which I ran on beta locally before requesting approval.  I also tested manually locally.

[Risks and why]:
If we didn't have test coverage, moderate risk, since this touches the code that handles searching on about:home/newtab.  But given that I tested this as I just described, I'd say low risk.  The patch is also pretty small.

[String/UUID change made/needed]:
None
Attachment #8513676 - Flags: review+
Attachment #8513676 - Flags: approval-mozilla-beta?
This is the patch that landed on m-c.  It differs from the previously posted patch in that it includes Matt's catch-cuddling nit from comment 18.

Approval Request Comment: Please see comment 21.  However, unlike in comment 21, I didn't bother running tests or manually testing locally because this patch applies cleanly to m-a and I wouldn't expect any behavior differences from m-c.
Attachment #8513678 - Flags: review+
Attachment #8513678 - Flags: approval-mozilla-aurora?
Attachment #8513678 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8513676 [details] [diff] [review]
Beta/34 patch

Beta+
Attachment #8513676 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed using Firefox 34 beta 7, latest Aurora 35.0a2 and latest Nightly 36.0a1 2014-11-06 under Win 7 64-bit, Ubuntu 14.04 64-bit and Mac OSX 10.9.5.
You need to log in before you can comment on or make changes to this bug.