Toady Reporter
	Description • 10 years ago

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

	Loic
	Comment 1 • 10 years ago

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?

	Toady Reporter
	Comment 2 • 10 years ago

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.

	Loic
	Comment 3 • 10 years ago

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

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 4 • 10 years ago

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?

	Matthew N. [:MattN]
	Comment 5 • 10 years ago

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.

	Toady Reporter
	Comment 6 • 10 years ago

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.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 7 • 10 years ago

In that case, this issue is not significant enough to track but is something that we would certainly take a fix for.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 9 • 10 years ago

This seems consistent with search.xml's behavior:
http://hg.mozilla.org/mozilla-central/annotate/c0ddb1b098ec/browser/components/search/content/search.xml#l483

	Drew Willcoxon :adw Assignee
	Comment 10 • 10 years ago

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.

	Drew Willcoxon :adw Assignee
	Comment 11 • 10 years ago

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. :-/

	Drew Willcoxon :adw Assignee
	Comment 12 • 10 years ago

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?

	Matthew N. [:MattN]
	Comment 13 • 10 years ago

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.

	Matthew N. [:MattN]
	Comment 14 • 10 years ago

FWIW, I also like adw's patch better because of where it's fixing the problem.

	Drew Willcoxon :adw Assignee
	Comment 15 • 10 years ago

(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.

	Drew Willcoxon :adw Assignee
	Comment 16 • 10 years ago

(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?

	Drew Willcoxon :adw Assignee
	Comment 17 • 10 years ago

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.

	Matthew N. [:MattN]
	Comment 18 • 10 years ago

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) {}

	Drew Willcoxon :adw Assignee
	Comment 19 • 10 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 20 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/8546940128b3

	Drew Willcoxon :adw Assignee
	Comment 21 • 10 years ago

(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

	Drew Willcoxon :adw Assignee
	Comment 22 • 10 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/272d21e92203

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 24 • 10 years ago

Comment on attachment 8513676 [details] [diff] [review]
Beta/34 patch

Beta+

	Drew Willcoxon :adw Assignee
	Comment 25 • 10 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/cadb1112c8fb

	Petruta Horea [:phorea], Desktop QA
	Comment 26 • 10 years ago

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.